[ESQL] Add warnings on STATS alias collisions (#115660) (#116248)

Fixes #114970

Added the warnings in the `RemoveStatsOverride` LogicalPlan rule, which is the same one that's removing the duplicates.

Also, fixed the groupings parser, which was assigning, to each stats grouping field, the source of the full "grouping context" instead. Without this fix, the warnings on groupings would, in some cases, say something like `Line 2:10: Field 'x' shadowed by field at line 2:10`.

As there are already tests for these cases, I'm requiring the capability on them, and updating their warnings expectations.

## Notes
I'm treating this as an enhancement instead of a bug. As there's existing logic removing duplicates, I'll guess this was decided at some point (Decision that may apply more or less nowadays).
And still, solving it this way is less dangerous and doesn't break compatibility.

Co-authored-by: Elastic Machine <[email protected]>

Author: ivancea

x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java

@@ -38,7 +38,6 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Optional;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -231,20 +230,13 @@ static CsvSpecReader.CsvTestCase convertToRemoteIndices(CsvSpecReader.CsvTestCas
         }
         int offset = testCase.query.length() - query.length();
         if (offset != 0) {
-            final String pattern = "Line (\\d+):(\\d+):";
+            final String pattern = "\\b1:(\\d+)\\b";
             final Pattern regex = Pattern.compile(pattern);
-            testCase.adjustExpectedWarnings(warning -> {
-                Matcher matcher = regex.matcher(warning);
-                if (matcher.find()) {
-                    int line = Integer.parseInt(matcher.group(1));
-                    if (line == 1) {
-                        int position = Integer.parseInt(matcher.group(2));
-                        int newPosition = position + offset;
-                        return warning.replaceFirst(pattern, "Line " + line + ":" + newPosition + ":");
-                    }
-                }
-                return warning;
-            });
+            testCase.adjustExpectedWarnings(warning -> regex.matcher(warning).replaceAll(match -> {
+                int position = Integer.parseInt(match.group(1));
+                int newPosition = position + offset;
+                return "1:" + newPosition;
+            }));
         }
         return testCase;
     }

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

@@ -577,6 +577,7 @@ abbrev:keyword | scalerank:integer |             location:geo_point
 
 byTwoCalculatedSecondOverwrites-Ignore
 required_capability: join_planning_v1
+required_capability: stats_alias_collision_warnings
 
 FROM airports
 | WHERE abbrev IS NOT NULL
@@ -587,6 +588,7 @@ FROM airports
 | SORT abbrev DESC
 | LIMIT 3
 ;
+warning:Line 5:4: Field 'x' shadowed by field at line 6:3
 
 abbrev:keyword | scalerank:integer |             location:geo_point             | x:double | min_sl:integer
            ZRH |                 3 | POINT(8.56221279534765 47.4523895064915)   |       10 | 2
@@ -596,6 +598,7 @@ abbrev:keyword | scalerank:integer |             location:geo_point
 
 byTwoCalculatedSecondOverwritesReferencingFirst-Ignore
 required_capability: join_planning_v1
+required_capability: stats_alias_collision_warnings
 
 FROM airports
 | WHERE abbrev IS NOT NULL
@@ -607,6 +610,7 @@ FROM airports
 | SORT abbrev DESC
 | LIMIT 3
 ;
+warning:Line 6:4: Field 'x' shadowed by field at line 7:3
 
 abbrev:keyword | scalerank:integer |             location:geo_point             | x:double | min_sl:integer
            ZRH |                 3 | POINT(8.56221279534765 47.4523895064915)   |       10 | 2
@@ -617,6 +621,7 @@ abbrev:keyword | scalerank:integer |             location:geo_point
 
 groupShadowsAgg-Ignore
 required_capability: join_planning_v1
+required_capability: stats_alias_collision_warnings
 
 FROM airports
 | WHERE abbrev IS NOT NULL
@@ -628,6 +633,7 @@ FROM airports
 | SORT abbrev DESC
 | LIMIT 3
 ;
+warning:Line 5:3: Field 'lat_10' shadowed by field at line 6:4
 
 abbrev:keyword | scalerank:integer |             location:geo_point             | lat_10:double | lon_10:double | min_sl:integer
            ZRH |                 3 | POINT(8.56221279534765 47.4523895064915)   |            50 |            10 | 2

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

@@ -509,7 +509,9 @@ m:d  | languages:i
 ; 
 
 IfDuplicateNamesLastOneWins
+required_capability: stats_alias_collision_warnings
 from employees | stats h = avg(height), h = min(height) by languages | sort languages;
+warning:Line 1:24: Field 'h' shadowed by field at line 1:41
 
 h:d  | languages:i
 1.42 | 1
@@ -533,7 +535,10 @@ m:d  | l:i
 ; 
 
 IfDuplicateNamesGroupingHasPriority
+required_capability: stats_alias_collision_warnings
 from employees | stats languages = avg(height), languages = min(height) by languages | sort languages;
+warning:Line 1:24: Field 'languages' shadowed by field at line 1:76
+warning:Line 1:49: Field 'languages' shadowed by field at line 1:76
 
 languages:i
 1
@@ -1582,11 +1587,13 @@ e: i  | f:i   | g:l | a:i
 ;
 
 nestedAggsOverGroupingTwiceWithAlias#[skip:-8.13.99,reason:supported in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS vals = COUNT() BY x = emp_no, x = languages
 | SORT x
 | LIMIT 3
 ;
+warning:Line 2:27: Field 'x' shadowed by field at line 2:39
 
 vals: l| x:i
 15     | 1
@@ -1623,11 +1630,13 @@ m:i | o:i   | l:i | s:i
 ;
 
 byTwoCalculatedSecondSameNameAsFirst#[skip:-8.13.99,reason:supported in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS m = MAX(salary) by l = salary + 1, l = languages + 1
 | SORT m
 | LIMIT 5
 ;
+warning:Line 2:28: Field 'l' shadowed by field at line 2:44
 
  m:i  | l:i
 66817 | 6
@@ -1638,12 +1647,14 @@ FROM employees
 ;
 
 byTwoCalculatedSecondShadowingAndReferencingFirst#[skip:-8.13.99,reason:supported in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | EVAL l = languages
 | STATS m = MAX(salary) by l = l + 1, l = l + 1
 | SORT m
 | LIMIT 5
 ;
+warning:Line 3:28: Field 'l' shadowed by field at line 3:39
 
  m:i  | l:i
 66817 | 6
@@ -2013,12 +2024,14 @@ c:l| languages:i
 ;
 
 evalMultipleOverridingKeys#[skip:-8.13.99,reason:supported in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | EVAL k = languages, k1 = k
 | STATS c = COUNT() BY languages, k, k1, languages
 | DROP k
 | SORT languages
 ;
+warning:Line 3:24: Field 'languages' shadowed by field at line 3:42
 
 c:l | k1:i | languages:i
 15  | 1    | 1
@@ -2030,12 +2043,14 @@ c:l | k1:i | languages:i
 ;
 
 evalMultipleOverridingKeysWithAggregateExpr#[skip:-8.13.99,reason:supported in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | EVAL k = languages, k1 = k
 | STATS c = 3*COUNT() BY languages, k, k1, languages
 | DROP k
 | SORT languages
 ;
+warning:Line 3:26: Field 'languages' shadowed by field at line 3:44
 
 c:l | k1:i | languages:i
 45  | 1    | 1
@@ -2193,19 +2208,24 @@ null
 ;
 
 shadowingInternal
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS x = MAX(emp_no), x = MIN(emp_no)
 ;
+warning:Line 2:9: Field 'x' shadowed by field at line 2:26
 
 x:integer
 10001
 ;
 
 shadowingInternalWithGroup#[skip:-8.14.1,reason:implemented in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS x = MAX(emp_no), x = MIN(emp_no) BY x = gender
 | SORT x ASC
 ;
+warning:Line 2:26: Field 'x' shadowed by field at line 2:45
+warning:Line 2:9: Field 'x' shadowed by field at line 2:45
 
 x:keyword
 F
@@ -2214,10 +2234,13 @@ null
 ;
 
 shadowingInternalWithGroup2#[skip:-8.14.1,reason:implemented in 8.14]
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS x = MAX(emp_no), y = count(x) BY x = emp_no, x = gender
 | SORT x ASC
 ;
+warning:Line 2:42: Field 'x' shadowed by field at line 2:54
+warning:Line 2:9: Field 'x' shadowed by field at line 2:54
 
 y:long | x:keyword
     33 | F
@@ -2227,10 +2250,13 @@ y:long | x:keyword
 
 
 shadowingTheGroup
+required_capability: stats_alias_collision_warnings
 FROM employees
 | STATS gender = MAX(emp_no), gender = MIN(emp_no) BY gender
 | SORT gender ASC
 ;
+warning:Line 2:31: Field 'gender' shadowed by field at line 2:55
+warning:Line 2:9: Field 'gender' shadowed by field at line 2:55
 
 gender:keyword
 F

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

@@ -433,6 +433,12 @@ public enum Cap {
          */
         FIX_FILTER_PUSHDOWN_PAST_STATS,
 
+        /**
+         * Send warnings on STATS alias collision
+         * https://github.com/elastic/elasticsearch/issues/114970
+         */
+        STATS_ALIAS_COLLISION_WARNINGS,
+
         /**
          * This enables 60_usage.yml "Basic ESQL usage....snapshot" version test. See also the next capability.
          */

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveStatsOverride.java

@@ -7,7 +7,7 @@
 
 package org.elasticsearch.xpack.esql.optimizer.rules.logical;
 
-import org.elasticsearch.common.util.set.Sets;
+import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.expression.Expressions;
 import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
@@ -16,6 +16,8 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import static org.elasticsearch.common.logging.HeaderWarning.addWarning;
+
 /**
  * Removes {@link Aggregate} overrides in grouping, aggregates and across them inside.
  * The overrides appear when the same alias is used multiple times in aggregations
@@ -42,13 +44,24 @@ protected LogicalPlan rule(Aggregate aggregate) {
 
     private static <T extends Expression> List<T> removeDuplicateNames(List<T> list) {
         var newList = new ArrayList<>(list);
-        var nameSet = Sets.newHashSetWithExpectedSize(list.size());
+        var expressionsByName = Maps.<String, T>newMapWithExpectedSize(list.size());
 
         // remove duplicates
         for (int i = list.size() - 1; i >= 0; i--) {
             var element = list.get(i);
             var name = Expressions.name(element);
-            if (nameSet.add(name) == false) {
+            var previousExpression = expressionsByName.putIfAbsent(name, element);
+            if (previousExpression != null) {
+                var source = element.source().source();
+                var previousSource = previousExpression.source().source();
+                addWarning(
+                    "Line {}:{}: Field '{}' shadowed by field at line {}:{}",
+                    source.getLineNumber(),
+                    source.getColumnNumber(),
+                    name,
+                    previousSource.getLineNumber(),
+                    previousSource.getColumnNumber()
+                );
                 newList.remove(i);
             }
         }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

@@ -811,7 +811,7 @@ public List<NamedExpression> visitGrouping(EsqlBaseParser.FieldsContext ctx) {
                 }
                 // wrap when necessary - no alias and no underlying attribute
                 if (ne == null) {
-                    ne = new Alias(source(ctx), name, value);
+                    ne = new Alias(source(field), name, value);
                 }
                 list.add(ne);
             }

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

@@ -536,13 +536,12 @@ public void testGroupingAlias() throws Exception {
 
     public void testGroupingAliasDuplicate() throws Exception {
         assertEquals(
-            "1:22: column [languages] cannot be used as an aggregate "
-                + "once declared in the STATS BY grouping key [l = languages % 3, l = languages, l = languages % 2]",
+            "1:22: column [languages] cannot be used as an aggregate once declared in the STATS BY grouping key [l = languages % 3]",
             error("from test| stats l = languages + 3 by l = languages % 3, l = languages, l = languages % 2 | keep l")
         );
 
         assertEquals(
-            "1:22: column [languages] cannot be used as an aggregate " + "once declared in the STATS BY grouping key [l = languages % 3]",
+            "1:22: column [languages] cannot be used as an aggregate once declared in the STATS BY grouping key [l = languages % 3]",
             error("from test| stats l = languages + 3, l = languages % 2  by l = languages % 3 | keep l")
         );
 

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

@@ -490,6 +490,11 @@ public void testRemoveOverridesInAggregate() throws Exception {
         var alias = as(aggregates.get(0), Alias.class);
         var max = as(alias.child(), Max.class);
         assertThat(Expressions.name(max.arguments().get(0)), equalTo("emp_no"));
+        assertWarnings(
+            "No limit defined, adding default limit of [1000]",
+            "Line 2:28: Field 'x' shadowed by field at line 2:45",
+            "Line 2:9: Field 'x' shadowed by field at line 2:45"
+        );
     }
 
     // expected stats b by b (grouping overrides the rest of the aggs)
@@ -512,6 +517,11 @@ public void testAggsWithOverridingInputAndGrouping() throws Exception {
         var aggregates = agg.aggregates();
         assertThat(aggregates, hasSize(1));
         assertThat(Expressions.names(aggregates), contains("b"));
+        assertWarnings(
+            "No limit defined, adding default limit of [1000]",
+            "Line 2:28: Field 'b' shadowed by field at line 2:47",
+            "Line 2:9: Field 'b' shadowed by field at line 2:47"
+        );
     }
 
     /**