diff --git a/x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java b/x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java index ba24668a5b29d..577158bb79269 100644 --- a/x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java +++ b/x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java @@ -36,4 +36,9 @@ protected boolean enableRoundingDoubleValuesOnAsserting() { protected boolean supportsSourceFieldMapping() { return false; } + + @Override + protected String maybeRandomizeQuery(String query) { + return randomlyNullify(query); + } } diff --git a/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java b/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java index 7793b39b02e57..e130c981c6373 100644 --- a/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java +++ b/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java @@ -55,13 +55,7 @@ protected boolean supportsSourceFieldMapping() { @Override protected String maybeRandomizeQuery(String query) { - // TODO we should implement more generic randomization for SET parameters - return randomBoolean() - && testCase.expectedWarnings().isEmpty() // avoid shifting warnings positions in source query - && testCase.expectedWarningsRegex().isEmpty() // regexp might also contain line/position - && query.startsWith("SET") == false // avoid conflicts with provided settings - ? "SET unmapped_fields=\"nullify\"; " + query - : query; + return randomlyNullify(query); } @Before diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java index 7a2c583ce2ef6..a87e49c332aa6 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java @@ -346,6 +346,19 @@ protected String maybeRandomizeQuery(String query) { return query; } + /** + * Intended to be used in {@link #maybeRandomizeQuery(String)} except in test cases that do not support {@code nullify} + * (e.g. old test cases in bwc tests) + */ + public String randomlyNullify(String query) { + return randomBoolean() + && testCase.expectedWarnings().isEmpty() // avoid shifting warnings positions in source query + && testCase.expectedWarningsRegex().isEmpty() // regexp might also contain line/position + && query.startsWith("SET") == false // avoid conflicts with provided settings + ? "SET unmapped_fields=\"nullify\"; " + query + : query; + } + /** * Returns true if the cluster under test supports the given ESQL capability. * Subclasses may override this to check additional clusters (e.g. remote clusters in CCS). diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped-nullify.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped-nullify.csv-spec index 345375f5eccfd..ab225c7e4376d 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped-nullify.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped-nullify.csv-spec @@ -12,12 +12,17 @@ null null ; +// Reproducer for https://github.com/elastic/elasticsearch/issues/141870 keepMapped -required_capability: date_nanos_type -required_capability: optional_fields_nullify_tech_preview required_capability: optional_fields_fix_unmapped_field_detection -SET unmapped_fields="nullify"; FROM date_nanos | SORT millis ASC | WHERE millis < "2000-01-01" | EVAL nanos = MV_MIN(nanos) | KEEP nanos; +SET unmapped_fields="nullify"\; +FROM date_nanos +| SORT millis ASC +| WHERE millis < "2000-01-01" +| EVAL nanos = MV_MIN(nanos) +| KEEP nanos +; nanos:date_nanos 2023-03-23T12:15:03.360103847Z diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java index e9db0faf34468..78292ab80a82d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java @@ -90,21 +90,8 @@ private static LogicalPlan resolve(LogicalPlan plan, boolean load) { if (plan.childrenResolved() == false) { return plan; } - var unresolved = collectUnresolved(plan); - if (unresolved.isEmpty()) { - return plan; - } - // Filter out unresolved attributes that exist in the children's output. These attributes are not truly unmapped; - // they just haven't been resolved yet by ResolveRefs (e.g. because the children only became resolved after ImplicitCasting). - // ResolveRefs will wire them up in the next iteration of the resolution batch. - Set childOutputNames = new java.util.HashSet<>(); - for (LogicalPlan child : plan.children()) { - for (Attribute attr : child.output()) { - childOutputNames.add(attr.name()); - } - } - unresolved.removeIf(ua -> childOutputNames.contains(ua.name())); + var unresolved = collectUnresolved(plan); if (unresolved.isEmpty()) { return plan; } @@ -362,13 +349,24 @@ private static Alias nullAlias(NamedExpression attribute) { * {@link UnresolvedTimestamp} subtypes. */ private static LinkedHashSet collectUnresolved(LogicalPlan plan) { + Set childOutputNames = new java.util.HashSet<>(); + for (LogicalPlan child : plan.children()) { + for (Attribute attr : child.output()) { + childOutputNames.add(attr.name()); + } + } Set aliasedGroupings = aliasNamesInAggregateGroupings(plan); + LinkedHashMap unresolved = new LinkedHashMap<>(); Consumer collectUnresolved = ua -> { if (leaveUnresolved(ua) == false // The aggs will "export" the aliases as UnresolvedAttributes part of their .aggregates(); we don't need to consider those // as they'll be resolved as refs once the aliased expression is resolved. - && aliasedGroupings.contains(ua.name()) == false) { + && aliasedGroupings.contains(ua.name()) == false + // Filter out unresolved attributes that exist in the children's output. These attributes are not truly unmapped; + // they just haven't been resolved yet by ResolveRefs (e.g. because the children only became resolved after ImplicitCasting). + // ResolveRefs will wire them up in the next iteration of the resolution batch. + && childOutputNames.contains(ua.name()) == false) { unresolved.putIfAbsent(ua.name(), ua); } }; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java index 20082b6ebacad..def62348489cd 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerUnmappedTests.java @@ -56,6 +56,7 @@ import java.util.List; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.asLimit; import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.analyzeStatement; import static org.elasticsearch.xpack.esql.analysis.AnalyzerTests.withInlinestatsWarning; @@ -3285,6 +3286,45 @@ public void testQstrAfterSortWithUnmappedField() { assertThat(orderBy, not(nullValue())); } + /* + * Reproducer for https://github.com/elastic/elasticsearch/issues/141870 + * ResolveRefs processes the EVAL only after ImplicitCasting processes the implicit cast in the WHERE. + * This means that ResolveUnmapped will see the EVAL with a yet-to-be-resolved reference to nanos. + * It should not treat it as unmapped, because there is clearly a nanos attribute in the EVAL's input. + * + * Limit[1000[INTEGER],false,false] + * \_Eval[[MVMIN(nanos{r}#6) AS nanos#11]] + * \_Filter[millis{r}#4 < 946684800000[DATETIME]] + * \_OrderBy[[Order[millis{r}#4,ASC,LAST]]] + * \_Row[[TODATETIME(1970-01-01T00:00:00Z[KEYWORD]) AS millis#4, TODATENANOS(1970-01-01T00:00:00Z[KEYWORD]) AS nanos#6]] + */ + public void testDoNotResolveUnmappedFieldPresentInChildren() { + String query = """ + ROW millis = "1970-01-01T00:00:00Z"::date, nanos = "1970-01-01T00:00:00Z"::date_nanos + | SORT millis ASC + | WHERE millis < "2000-01-01" + | EVAL nanos = MV_MIN(nanos) + """; + boolean useNullify = randomBoolean(); + var plan = analyzeStatement(useNullify ? setUnmappedNullify(query) : setUnmappedLoad(query)); + + var limit = asLimit(plan, 1000); + var eval = as(limit.child(), Eval.class); + var filter = as(eval.child(), Filter.class); + var orderBy = as(filter.child(), OrderBy.class); + // There should be no EVAL injected with NULL for nanos + var row = as(orderBy.child(), Row.class); + + var output = plan.output(); + assertThat(output, hasSize(2)); + var millisAttr = output.get(0); + var nanosAttr = output.get(1); + assertThat(millisAttr.name(), is("millis")); + assertThat(millisAttr.dataType(), is(DataType.DATETIME)); + assertThat(nanosAttr.name(), is("nanos")); + assertThat(nanosAttr.dataType(), is(DataType.DATE_NANOS)); + } + private void verificationFailure(String statement, String expectedFailure) { var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement)); assertThat(e.getMessage(), containsString(expectedFailure));