ESQL: Small nullify fix cleanups

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);
+    }
 }

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

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).

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

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<String> 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<UnresolvedAttribute> collectUnresolved(LogicalPlan plan) {
+        Set<String> childOutputNames = new java.util.HashSet<>();
+        for (LogicalPlan child : plan.children()) {
+            for (Attribute attr : child.output()) {
+                childOutputNames.add(attr.name());
+            }
+        }
         Set<String> aliasedGroupings = aliasNamesInAggregateGroupings(plan);
+
         LinkedHashMap<String, UnresolvedAttribute> unresolved = new LinkedHashMap<>();
         Consumer<UnresolvedAttribute> 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);
             }
         };

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));