Skip to content

Commit bc4574c

Browse files
ivanceaivancea
authored
[ESQL] Add warnings on STATS alias collisions (#115660)
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.
1 parent 954ab8a commit bc4574c

File tree

8 files changed

+73
-21
lines changed

8 files changed

+73
-21
lines changed

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

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import java.util.List;
3939
import java.util.Locale;
4040
import java.util.Optional;
41-
import java.util.regex.Matcher;
4241
import java.util.regex.Pattern;
4342
import java.util.stream.Collectors;
4443

@@ -231,20 +230,13 @@ static CsvSpecReader.CsvTestCase convertToRemoteIndices(CsvSpecReader.CsvTestCas
231230
}
232231
int offset = testCase.query.length() - query.length();
233232
if (offset != 0) {
234-
final String pattern = "Line (\\d+):(\\d+):";
233+
final String pattern = "\\b1:(\\d+)\\b";
235234
final Pattern regex = Pattern.compile(pattern);
236-
testCase.adjustExpectedWarnings(warning -> {
237-
Matcher matcher = regex.matcher(warning);
238-
if (matcher.find()) {
239-
int line = Integer.parseInt(matcher.group(1));
240-
if (line == 1) {
241-
int position = Integer.parseInt(matcher.group(2));
242-
int newPosition = position + offset;
243-
return warning.replaceFirst(pattern, "Line " + line + ":" + newPosition + ":");
244-
}
245-
}
246-
return warning;
247-
});
235+
testCase.adjustExpectedWarnings(warning -> regex.matcher(warning).replaceAll(match -> {
236+
int position = Integer.parseInt(match.group(1));
237+
int newPosition = position + offset;
238+
return "1:" + newPosition;
239+
}));
248240
}
249241
return testCase;
250242
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,7 @@ abbrev:keyword | scalerank:integer | location:geo_point
577577

578578
byTwoCalculatedSecondOverwrites-Ignore
579579
required_capability: join_planning_v1
580+
required_capability: stats_alias_collision_warnings
580581

581582
FROM airports
582583
| WHERE abbrev IS NOT NULL
@@ -587,6 +588,7 @@ FROM airports
587588
| SORT abbrev DESC
588589
| LIMIT 3
589590
;
591+
warning:Line 5:4: Field 'x' shadowed by field at line 6:3
590592

591593
abbrev:keyword | scalerank:integer | location:geo_point | x:double | min_sl:integer
592594
ZRH | 3 | POINT(8.56221279534765 47.4523895064915) | 10 | 2
@@ -596,6 +598,7 @@ abbrev:keyword | scalerank:integer | location:geo_point
596598

597599
byTwoCalculatedSecondOverwritesReferencingFirst-Ignore
598600
required_capability: join_planning_v1
601+
required_capability: stats_alias_collision_warnings
599602

600603
FROM airports
601604
| WHERE abbrev IS NOT NULL
@@ -607,6 +610,7 @@ FROM airports
607610
| SORT abbrev DESC
608611
| LIMIT 3
609612
;
613+
warning:Line 6:4: Field 'x' shadowed by field at line 7:3
610614

611615
abbrev:keyword | scalerank:integer | location:geo_point | x:double | min_sl:integer
612616
ZRH | 3 | POINT(8.56221279534765 47.4523895064915) | 10 | 2
@@ -617,6 +621,7 @@ abbrev:keyword | scalerank:integer | location:geo_point
617621

618622
groupShadowsAgg-Ignore
619623
required_capability: join_planning_v1
624+
required_capability: stats_alias_collision_warnings
620625

621626
FROM airports
622627
| WHERE abbrev IS NOT NULL
@@ -628,6 +633,7 @@ FROM airports
628633
| SORT abbrev DESC
629634
| LIMIT 3
630635
;
636+
warning:Line 5:3: Field 'lat_10' shadowed by field at line 6:4
631637

632638
abbrev:keyword | scalerank:integer | location:geo_point | lat_10:double | lon_10:double | min_sl:integer
633639
ZRH | 3 | POINT(8.56221279534765 47.4523895064915) | 50 | 10 | 2

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,9 @@ m:d | languages:i
509509
;
510510

511511
IfDuplicateNamesLastOneWins
512+
required_capability: stats_alias_collision_warnings
512513
from employees | stats h = avg(height), h = min(height) by languages | sort languages;
514+
warning:Line 1:24: Field 'h' shadowed by field at line 1:41
513515

514516
h:d | languages:i
515517
1.42 | 1
@@ -533,7 +535,10 @@ m:d | l:i
533535
;
534536

535537
IfDuplicateNamesGroupingHasPriority
538+
required_capability: stats_alias_collision_warnings
536539
from employees | stats languages = avg(height), languages = min(height) by languages | sort languages;
540+
warning:Line 1:24: Field 'languages' shadowed by field at line 1:76
541+
warning:Line 1:49: Field 'languages' shadowed by field at line 1:76
537542

538543
languages:i
539544
1
@@ -1582,11 +1587,13 @@ e: i | f:i | g:l | a:i
15821587
;
15831588

15841589
nestedAggsOverGroupingTwiceWithAlias#[skip:-8.13.99,reason:supported in 8.14]
1590+
required_capability: stats_alias_collision_warnings
15851591
FROM employees
15861592
| STATS vals = COUNT() BY x = emp_no, x = languages
15871593
| SORT x
15881594
| LIMIT 3
15891595
;
1596+
warning:Line 2:27: Field 'x' shadowed by field at line 2:39
15901597

15911598
vals: l| x:i
15921599
15 | 1
@@ -1623,11 +1630,13 @@ m:i | o:i | l:i | s:i
16231630
;
16241631

16251632
byTwoCalculatedSecondSameNameAsFirst#[skip:-8.13.99,reason:supported in 8.14]
1633+
required_capability: stats_alias_collision_warnings
16261634
FROM employees
16271635
| STATS m = MAX(salary) by l = salary + 1, l = languages + 1
16281636
| SORT m
16291637
| LIMIT 5
16301638
;
1639+
warning:Line 2:28: Field 'l' shadowed by field at line 2:44
16311640

16321641
m:i | l:i
16331642
66817 | 6
@@ -1638,12 +1647,14 @@ FROM employees
16381647
;
16391648

16401649
byTwoCalculatedSecondShadowingAndReferencingFirst#[skip:-8.13.99,reason:supported in 8.14]
1650+
required_capability: stats_alias_collision_warnings
16411651
FROM employees
16421652
| EVAL l = languages
16431653
| STATS m = MAX(salary) by l = l + 1, l = l + 1
16441654
| SORT m
16451655
| LIMIT 5
16461656
;
1657+
warning:Line 3:28: Field 'l' shadowed by field at line 3:39
16471658

16481659
m:i | l:i
16491660
66817 | 6
@@ -2013,12 +2024,14 @@ c:l| languages:i
20132024
;
20142025

20152026
evalMultipleOverridingKeys#[skip:-8.13.99,reason:supported in 8.14]
2027+
required_capability: stats_alias_collision_warnings
20162028
FROM employees
20172029
| EVAL k = languages, k1 = k
20182030
| STATS c = COUNT() BY languages, k, k1, languages
20192031
| DROP k
20202032
| SORT languages
20212033
;
2034+
warning:Line 3:24: Field 'languages' shadowed by field at line 3:42
20222035

20232036
c:l | k1:i | languages:i
20242037
15 | 1 | 1
@@ -2030,12 +2043,14 @@ c:l | k1:i | languages:i
20302043
;
20312044

20322045
evalMultipleOverridingKeysWithAggregateExpr#[skip:-8.13.99,reason:supported in 8.14]
2046+
required_capability: stats_alias_collision_warnings
20332047
FROM employees
20342048
| EVAL k = languages, k1 = k
20352049
| STATS c = 3*COUNT() BY languages, k, k1, languages
20362050
| DROP k
20372051
| SORT languages
20382052
;
2053+
warning:Line 3:26: Field 'languages' shadowed by field at line 3:44
20392054

20402055
c:l | k1:i | languages:i
20412056
45 | 1 | 1
@@ -2193,19 +2208,24 @@ null
21932208
;
21942209

21952210
shadowingInternal
2211+
required_capability: stats_alias_collision_warnings
21962212
FROM employees
21972213
| STATS x = MAX(emp_no), x = MIN(emp_no)
21982214
;
2215+
warning:Line 2:9: Field 'x' shadowed by field at line 2:26
21992216

22002217
x:integer
22012218
10001
22022219
;
22032220

22042221
shadowingInternalWithGroup#[skip:-8.14.1,reason:implemented in 8.14]
2222+
required_capability: stats_alias_collision_warnings
22052223
FROM employees
22062224
| STATS x = MAX(emp_no), x = MIN(emp_no) BY x = gender
22072225
| SORT x ASC
22082226
;
2227+
warning:Line 2:26: Field 'x' shadowed by field at line 2:45
2228+
warning:Line 2:9: Field 'x' shadowed by field at line 2:45
22092229

22102230
x:keyword
22112231
F
@@ -2214,10 +2234,13 @@ null
22142234
;
22152235

22162236
shadowingInternalWithGroup2#[skip:-8.14.1,reason:implemented in 8.14]
2237+
required_capability: stats_alias_collision_warnings
22172238
FROM employees
22182239
| STATS x = MAX(emp_no), y = count(x) BY x = emp_no, x = gender
22192240
| SORT x ASC
22202241
;
2242+
warning:Line 2:42: Field 'x' shadowed by field at line 2:54
2243+
warning:Line 2:9: Field 'x' shadowed by field at line 2:54
22212244

22222245
y:long | x:keyword
22232246
33 | F
@@ -2227,10 +2250,13 @@ y:long | x:keyword
22272250

22282251

22292252
shadowingTheGroup
2253+
required_capability: stats_alias_collision_warnings
22302254
FROM employees
22312255
| STATS gender = MAX(emp_no), gender = MIN(emp_no) BY gender
22322256
| SORT gender ASC
22332257
;
2258+
warning:Line 2:31: Field 'gender' shadowed by field at line 2:55
2259+
warning:Line 2:9: Field 'gender' shadowed by field at line 2:55
22342260

22352261
gender:keyword
22362262
F

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,12 @@ public enum Cap {
428428
*/
429429
FIX_FILTER_PUSHDOWN_PAST_STATS,
430430

431+
/**
432+
* Send warnings on STATS alias collision
433+
* https://github.com/elastic/elasticsearch/issues/114970
434+
*/
435+
STATS_ALIAS_COLLISION_WARNINGS,
436+
431437
/**
432438
* This enables 60_usage.yml "Basic ESQL usage....snapshot" version test. See also the next capability.
433439
*/

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
package org.elasticsearch.xpack.esql.optimizer.rules.logical;
99

10-
import org.elasticsearch.common.util.set.Sets;
10+
import org.elasticsearch.common.util.Maps;
1111
import org.elasticsearch.xpack.esql.core.expression.Expression;
1212
import org.elasticsearch.xpack.esql.core.expression.Expressions;
1313
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
@@ -16,6 +16,8 @@
1616
import java.util.ArrayList;
1717
import java.util.List;
1818

19+
import static org.elasticsearch.common.logging.HeaderWarning.addWarning;
20+
1921
/**
2022
* Removes {@link Aggregate} overrides in grouping, aggregates and across them inside.
2123
* The overrides appear when the same alias is used multiple times in aggregations
@@ -42,13 +44,24 @@ protected LogicalPlan rule(Aggregate aggregate) {
4244

4345
private static <T extends Expression> List<T> removeDuplicateNames(List<T> list) {
4446
var newList = new ArrayList<>(list);
45-
var nameSet = Sets.newHashSetWithExpectedSize(list.size());
47+
var expressionsByName = Maps.<String, T>newMapWithExpectedSize(list.size());
4648

4749
// remove duplicates
4850
for (int i = list.size() - 1; i >= 0; i--) {
4951
var element = list.get(i);
5052
var name = Expressions.name(element);
51-
if (nameSet.add(name) == false) {
53+
var previousExpression = expressionsByName.putIfAbsent(name, element);
54+
if (previousExpression != null) {
55+
var source = element.source().source();
56+
var previousSource = previousExpression.source().source();
57+
addWarning(
58+
"Line {}:{}: Field '{}' shadowed by field at line {}:{}",
59+
source.getLineNumber(),
60+
source.getColumnNumber(),
61+
name,
62+
previousSource.getLineNumber(),
63+
previousSource.getColumnNumber()
64+
);
5265
newList.remove(i);
5366
}
5467
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@ public List<NamedExpression> visitGrouping(EsqlBaseParser.FieldsContext ctx) {
815815
}
816816
// wrap when necessary - no alias and no underlying attribute
817817
if (ne == null) {
818-
ne = new Alias(source(ctx), name, value);
818+
ne = new Alias(source(field), name, value);
819819
}
820820
list.add(ne);
821821
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -523,13 +523,12 @@ public void testGroupingAlias() throws Exception {
523523

524524
public void testGroupingAliasDuplicate() throws Exception {
525525
assertEquals(
526-
"1:22: column [languages] cannot be used as an aggregate "
527-
+ "once declared in the STATS BY grouping key [l = languages % 3, l = languages, l = languages % 2]",
526+
"1:22: column [languages] cannot be used as an aggregate once declared in the STATS BY grouping key [l = languages % 3]",
528527
error("from test| stats l = languages + 3 by l = languages % 3, l = languages, l = languages % 2 | keep l")
529528
);
530529

531530
assertEquals(
532-
"1:22: column [languages] cannot be used as an aggregate " + "once declared in the STATS BY grouping key [l = languages % 3]",
531+
"1:22: column [languages] cannot be used as an aggregate once declared in the STATS BY grouping key [l = languages % 3]",
533532
error("from test| stats l = languages + 3, l = languages % 2 by l = languages % 3 | keep l")
534533
);
535534

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,11 @@ public void testRemoveOverridesInAggregate() throws Exception {
491491
var alias = as(aggregates.get(0), Alias.class);
492492
var max = as(alias.child(), Max.class);
493493
assertThat(Expressions.name(max.arguments().get(0)), equalTo("emp_no"));
494+
assertWarnings(
495+
"No limit defined, adding default limit of [1000]",
496+
"Line 2:28: Field 'x' shadowed by field at line 2:45",
497+
"Line 2:9: Field 'x' shadowed by field at line 2:45"
498+
);
494499
}
495500

496501
// expected stats b by b (grouping overrides the rest of the aggs)
@@ -513,6 +518,11 @@ public void testAggsWithOverridingInputAndGrouping() throws Exception {
513518
var aggregates = agg.aggregates();
514519
assertThat(aggregates, hasSize(1));
515520
assertThat(Expressions.names(aggregates), contains("b"));
521+
assertWarnings(
522+
"No limit defined, adding default limit of [1000]",
523+
"Line 2:28: Field 'b' shadowed by field at line 2:47",
524+
"Line 2:9: Field 'b' shadowed by field at line 2:47"
525+
);
516526
}
517527

518528
/**

0 commit comments

Comments
 (0)