ESQL - date nanos range bug? by not-napoleon · Pull Request #125345 · elastic/elasticsearch

not-napoleon · 2025-03-20T20:10:53Z

We were incorrectly formatting nanosecond dates when building lucene queries. We had missed this in our testing because none of the CSV tests were running against Lucene. This happened because the date nanos test data includes multivalue fields. Our warning behavior for multivalue fields is inconsistent between queries run in Lucene and queries run in pure ES|QL without pushdown. Our warning tests, however, require that the specified warnings be present in all execution paths. When we first built the date nanos CSV tests, we worked around this by always using an MV function to unpack the multivalue fields. But we forgot that using an MV function prevents the entire query from being pushed down to Lucene, and thus that path wasn't being tested.

In this PR, I've duplicated many of the tests to have a version that doesn't use the MV function, and uses warningRegex instead of warning. The regex version does not fail if the header is absent, so it's safe to use in both modes. Rewriting the tests this way revealed several situations in which this bug can manifest, all of which are fixed in this PR. I cannot be confidant that there aren't more paths that can trigger this bug and aren't covered by these tests, but I haven't found any yet.

I've left some trace level logging that I found helpful while debugging this.

elasticsearchmachine · 2025-03-24T16:26:51Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2025-03-24T16:26:52Z

Hi @not-napoleon, I've created a changelog YAML for you.

elasticsearchmachine · 2025-03-24T16:58:42Z

Hi @not-napoleon, I've updated the changelog YAML for you.

nik9000

I left some small stuff about the trace logs.

nik9000 · 2025-03-24T18:59:13Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

         */
        DATE_NANOS_DATE_DIFF(),
+        /**
+         * Indicates that https://github.com/elastic/elasticsearch/issues/125439 is fixed


I'd prefer if we added a short description so I don't have to open github when reading the code.

nik9000 · 2025-03-24T19:00:03Z

.../elasticsearch/xpack/esql/expression/predicate/operator/comparison/EsqlBinaryComparison.java

+                + attribute
+                + "<"
+                + attribute.dataType()
+                + ">]"


Could you do this with {} so we don't build the string every time?

nik9000 · 2025-03-24T19:00:18Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/Range.java

        DataType dataType = value.dataType();
-        if (DataType.isDateTime(dataType) && DataType.isDateTime(lower.dataType()) && DataType.isDateTime(upper.dataType())) {
+        logger.trace(
+            "Translating Range into lucene query.  dataType is [" + dataType + "] upper is [" + upper + "]  lower is [" + lower + "]"


Could you do this with {} so we don't build the string every time?

nik9000 · 2025-03-24T19:00:49Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java

    @Override
    public final PhysicalOperation sourcePhysicalOperation(EsQueryExec esQueryExec, LocalExecutionPlannerContext context) {
        final LuceneOperator.Factory luceneFactory;
+        logger.trace("Query Exec is " + esQueryExec);


Same comment as above.

fang-xing-esql

LGTM, thank you for fixing it quickly @not-napoleon! I added a couple of comments, they can be addressed as follow ups.

fang-xing-esql · 2025-03-24T19:21:44Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/date_nanos.csv-spec

 -18489600        | -18489599        | 2023-03-23T12:15:03.360103847Z
 ;
+
+Regression out of bounds in where clause


Some more tests that I think could be helpful are having both lower and upper bounds with literals that can disqualify rows, more combinations of >, >=, < and <=, for example

WHERE nanos > to_datenanos("2023-10-23T12:15:00") AND nanos <= to_datenanos("2023-10-23T13:53:55.832987654Z") WHERE nanos >= to_datenanos("2023-10-23T12:15:00") AND nanos <= now()

fang-xing-esql · 2025-03-24T19:25:28Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/predicate/Range.java

 import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.versionToString;

 // BETWEEN or range - is a mix of gt(e) AND lt(e)
 public class Range extends ScalarFunction implements TranslationAware.SingleValueTranslationAware {


There is a method areBoundariesInvalid to validate the lower and upper bounds, date_nanos can be added in there too, this can be a follow up.

…os-range-query-bug' into esql-date-nanos-range-query-bug

not-napoleon · 2025-03-24T21:39:50Z

@elasticmachine run elasticsearch-ci/part-2

Fixes elastic#125439 We were incorrectly formatting nanosecond dates when building lucene queries. We had missed this in our testing because none of the CSV tests were running against Lucene. This happened because the date nanos test data includes multivalue fields. Our warning behavior for multivalue fields is inconsistent between queries run in Lucene and queries run in pure ES|QL without pushdown. Our warning tests, however, require that the specified warnings be present in all execution paths. When we first built the date nanos CSV tests, we worked around this by always using an MV function to unpack the multivalue fields. But we forgot that using an MV function prevents the entire query from being pushed down to Lucene, and thus that path wasn't being tested. In this PR, I've duplicated many of the tests to have a version that doesn't use the MV function, and uses warningRegex instead of warning. The regex version does not fail if the header is absent, so it's safe to use in both modes. Rewriting the tests this way revealed several situations in which this bug can manifest, all of which are fixed in this PR. I cannot be confidant that there aren't more paths that can trigger this bug and aren't covered by these tests, but I haven't found any yet. I've left some trace level logging that I found helpful while debugging this. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

elasticsearchmachine · 2025-03-24T22:25:30Z

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	8.18
✅	8.x

Fixes elastic#125439 We were incorrectly formatting nanosecond dates when building lucene queries. We had missed this in our testing because none of the CSV tests were running against Lucene. This happened because the date nanos test data includes multivalue fields. Our warning behavior for multivalue fields is inconsistent between queries run in Lucene and queries run in pure ES|QL without pushdown. Our warning tests, however, require that the specified warnings be present in all execution paths. When we first built the date nanos CSV tests, we worked around this by always using an MV function to unpack the multivalue fields. But we forgot that using an MV function prevents the entire query from being pushed down to Lucene, and thus that path wasn't being tested. In this PR, I've duplicated many of the tests to have a version that doesn't use the MV function, and uses warningRegex instead of warning. The regex version does not fail if the header is absent, so it's safe to use in both modes. Rewriting the tests this way revealed several situations in which this bug can manifest, all of which are fixed in this PR. I cannot be confidant that there aren't more paths that can trigger this bug and aren't covered by these tests, but I haven't found any yet. I've left some trace level logging that I found helpful while debugging this. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

Fixes #125439 We were incorrectly formatting nanosecond dates when building lucene queries. We had missed this in our testing because none of the CSV tests were running against Lucene. This happened because the date nanos test data includes multivalue fields. Our warning behavior for multivalue fields is inconsistent between queries run in Lucene and queries run in pure ES|QL without pushdown. Our warning tests, however, require that the specified warnings be present in all execution paths. When we first built the date nanos CSV tests, we worked around this by always using an MV function to unpack the multivalue fields. But we forgot that using an MV function prevents the entire query from being pushed down to Lucene, and thus that path wasn't being tested. In this PR, I've duplicated many of the tests to have a version that doesn't use the MV function, and uses warningRegex instead of warning. The regex version does not fail if the header is absent, so it's safe to use in both modes. Rewriting the tests this way revealed several situations in which this bug can manifest, all of which are fixed in this PR. I cannot be confidant that there aren't more paths that can trigger this bug and aren't covered by these tests, but I haven't found any yet. I've left some trace level logging that I found helpful while debugging this. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (#125595) Follow up to #125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (elastic#125595) Follow up to elastic#125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (#125595) (#125617) Follow up to #125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (#125595) (#125618) Follow up to #125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (#125595) (#125619) Follow up to #125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

Fixes elastic#125439 We were incorrectly formatting nanosecond dates when building lucene queries. We had missed this in our testing because none of the CSV tests were running against Lucene. This happened because the date nanos test data includes multivalue fields. Our warning behavior for multivalue fields is inconsistent between queries run in Lucene and queries run in pure ES|QL without pushdown. Our warning tests, however, require that the specified warnings be present in all execution paths. When we first built the date nanos CSV tests, we worked around this by always using an MV function to unpack the multivalue fields. But we forgot that using an MV function prevents the entire query from being pushed down to Lucene, and thus that path wasn't being tested. In this PR, I've duplicated many of the tests to have a version that doesn't use the MV function, and uses warningRegex instead of warning. The regex version does not fail if the header is absent, so it's safe to use in both modes. Rewriting the tests this way revealed several situations in which this bug can manifest, all of which are fixed in this PR. I cannot be confidant that there aren't more paths that can trigger this bug and aren't covered by these tests, but I haven't found any yet. I've left some trace level logging that I found helpful while debugging this. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…millis (elastic#125595) Follow up to elastic#125345. If the query contained both a nanos and a millis comparison, we were formatting the dates incorrectly for the lucene push down. This PR adds a test and a fix for that case. --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

non-mv versions of date nanos tests

f0dfea6

not-napoleon added >bug :Analytics/ES|QL AKA ESQL v9.1.0 labels Mar 20, 2025

not-napoleon and others added 8 commits March 20, 2025 16:14

didn't mean to clobber some docs

a934963

ignore order where appropriate

59da8f4

fix several places this bug can come from

106d730

make warnings regexes, which are optional, apparently?

6892a01

fix value formatting for binary comparisons

97ccfe3

tell IN that DateNanos needs specific value handling

94a668b

clean up logging

b629514

[CI] Auto commit changes from spotless

5f4f830

not-napoleon added v9.0.0 v8.18.0 auto-backport Automatically create backport pull requests when merged labels Mar 24, 2025

Merge branch 'main' into esql-date-nanos-range-query-bug

004758c

not-napoleon marked this pull request as ready for review March 24, 2025 16:26

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 24, 2025

not-napoleon added 2 commits March 24, 2025 12:26

Update docs/changelog/125345.yaml

50d9093

Update docs/changelog/125345.yaml

3807994

not-napoleon added 2 commits March 24, 2025 14:08

Merge branch 'main' into esql-date-nanos-range-query-bug

a91f2cc

add capability check

212a4f5

not-napoleon enabled auto-merge (squash) March 24, 2025 18:48

costin added the v8.19.0 label Mar 24, 2025

nik9000 approved these changes Mar 24, 2025

View reviewed changes

missed one for the capability

3f40c8c

not-napoleon disabled auto-merge March 24, 2025 19:20

fang-xing-esql approved these changes Mar 24, 2025

View reviewed changes

not-napoleon and others added 4 commits March 24, 2025 15:47

clean up logging some more

aa7c89b

Merge branch 'main' into esql-date-nanos-range-query-bug

056e106

Merge remote-tracking branch 'refs/remotes/not-napoleon/esql-date-nan…

3a032df

…os-range-query-bug' into esql-date-nanos-range-query-bug

[CI] Auto commit changes from spotless

5da39d2

not-napoleon enabled auto-merge (squash) March 24, 2025 21:46

not-napoleon merged commit 30a56d6 into elastic:main Mar 24, 2025
8 of 17 checks passed

This was referenced Mar 24, 2025

[9.0] ESQL - date nanos range bug? (#125345) #125539

Merged

[8.18] ESQL - date nanos range bug? (#125345) #125540

Merged

not-napoleon mentioned this pull request Mar 24, 2025

[8.x] ESQL - date nanos range bug? (#125345) #125541

Merged

not-napoleon mentioned this pull request Mar 25, 2025

Esql - Fix lucene push down behavior when a range contains nanos and millis #125595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL - date nanos range bug? #125345

ESQL - date nanos range bug? #125345
not-napoleon merged 19 commits intoelastic:mainfrom
not-napoleon:esql-date-nanos-range-query-bug

not-napoleon commented Mar 20, 2025 •

edited

Loading

elasticsearchmachine commented Mar 24, 2025

elasticsearchmachine commented Mar 24, 2025

elasticsearchmachine commented Mar 24, 2025

nik9000 left a comment

nik9000 Mar 24, 2025

nik9000 Mar 24, 2025

nik9000 Mar 24, 2025

nik9000 Mar 24, 2025

fang-xing-esql left a comment

fang-xing-esql Mar 24, 2025

fang-xing-esql Mar 24, 2025

not-napoleon commented Mar 24, 2025

Uh oh!

elasticsearchmachine commented Mar 24, 2025

Labels

5 participants

Conversation

not-napoleon commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Mar 24, 2025

elasticsearchmachine commented Mar 24, 2025

elasticsearchmachine commented Mar 24, 2025

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Mar 24, 2025

Choose a reason for hiding this comment

nik9000 Mar 24, 2025

Choose a reason for hiding this comment

nik9000 Mar 24, 2025

Choose a reason for hiding this comment

nik9000 Mar 24, 2025

Choose a reason for hiding this comment

fang-xing-esql left a comment

Choose a reason for hiding this comment

fang-xing-esql Mar 24, 2025

Choose a reason for hiding this comment

fang-xing-esql Mar 24, 2025

Choose a reason for hiding this comment

not-napoleon commented Mar 24, 2025

Uh oh!

elasticsearchmachine commented Mar 24, 2025

💚 Backport successful

Labels

5 participants

not-napoleon commented Mar 20, 2025 •

edited

Loading