Skip to content

Commit bcbba30

Browse files
emasabmatriv
emasab
authored and
matriv
committed
SQL: Failing Group By queries due to different ExpressionIds (#43072)
Fix an issue that arises from the use of ExpressionIds as keys in a lookup map that helps the QueryTranslator to identify the grouping columns. The issue is that the same expression in different parts of the query (SELECT clause and GROUP BY clause) ends up with different ExpressionIds so the lookup fails. So, instead of ExpressionIds use the hashCode() of NamedExpression. Fixes: #41159 Fixes: #40001 Fixes: #40240 Fixes: #33361 Fixes: #46316 Fixes: #36074 Fixes: #34543 Fixes: #37044 Fixes: #42041 (cherry picked from commit 3c38ea5)
1 parent 1679a5c commit bcbba30

File tree

25 files changed

+609
-105
lines changed

25 files changed

+609
-105
lines changed

‎x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec

+188
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,194 @@ TRUNCATE(YEAR("birth_date"), -2)
244244
null
245245
1900
246246
;
247+
// Fails for H2
248+
groupByCastScalarWithNumericRef
249+
SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST;
250+
251+
CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT):l
252+
------------------------------------------------------
253+
null
254+
1952
255+
1953
256+
1954
257+
1955
258+
1956
259+
1957
260+
1958
261+
1959
262+
1960
263+
1961
264+
1962
265+
1963
266+
1964
267+
1965
268+
;
269+
270+
groupByConvertScalar
271+
SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) FROM test_emp GROUP BY CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) ORDER BY CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) NULLS FIRST;
272+
273+
274+
CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l
275+
-----------------------------------------------------------
276+
null
277+
1952
278+
1953
279+
1954
280+
1955
281+
1956
282+
1957
283+
1958
284+
1959
285+
1960
286+
1961
287+
1962
288+
1963
289+
1964
290+
1965
291+
;
292+
293+
294+
groupByConvertScalarWithAlias
295+
SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) as "convert" FROM test_emp GROUP BY "convert" ORDER BY "convert" NULLS FIRST;
296+
297+
convert:l
298+
---------
299+
null
300+
1952
301+
1953
302+
1954
303+
1955
304+
1956
305+
1957
306+
1958
307+
1959
308+
1960
309+
1961
310+
1962
311+
1963
312+
1964
313+
1965
314+
;
315+
316+
groupByConvertScalarWithNumericRef
317+
SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST;
318+
319+
CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l
320+
-----------------------------------------------------------
321+
null
322+
1952
323+
1953
324+
1954
325+
1955
326+
1956
327+
1957
328+
1958
329+
1959
330+
1960
331+
1961
332+
1962
333+
1963
334+
1964
335+
1965
336+
;
337+
338+
groupByConstantScalar
339+
SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10;
340+
341+
PI() * emp_no:d
342+
---------------
343+
31419.0681285515
344+
31422.2097212051
345+
31425.3513138587
346+
31428.4929065123
347+
31431.6344991659
348+
31434.7760918195
349+
31437.9176844731
350+
31441.0592771266
351+
31444.2008697802
352+
31447.3424624338
353+
;
354+
355+
groupByConstantScalarWithOrderByDesc
356+
SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no DESC LIMIT 10;
357+
358+
PI() * emp_no:d
359+
-------
360+
31730.0858012569
361+
31726.9442086033
362+
31723.8026159497
363+
31720.6610232961
364+
31717.5194306425
365+
31714.3778379889
366+
31711.2362453353
367+
31708.0946526817
368+
31704.9530600281
369+
31701.8114673746
370+
;
371+
372+
groupByConstantScalarWithAlias
373+
SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10;
374+
375+
value:d
376+
-------
377+
31419.0681285515
378+
31422.2097212051
379+
31425.3513138587
380+
31428.4929065123
381+
31431.6344991659
382+
31434.7760918195
383+
31437.9176844731
384+
31441.0592771266
385+
31444.2008697802
386+
31447.3424624338
387+
;
388+
389+
groupByConstantScalarWithNumericRef
390+
SELECT PI() * emp_no FROM test_emp GROUP BY 1 ORDER BY 1 DESC LIMIT 10;
391+
392+
PI() * emp_no:d
393+
-------
394+
31730.0858012569
395+
31726.9442086033
396+
31723.8026159497
397+
31720.6610232961
398+
31717.5194306425
399+
31714.3778379889
400+
31711.2362453353
401+
31708.0946526817
402+
31704.9530600281
403+
31701.8114673746
404+
;
405+
406+
groupByFieldAndConstantScalarWithMultipleOrderBy
407+
SELECT gender, emp_no % 3 + PI() FROM test_emp GROUP BY gender, emp_no % 3 + PI() ORDER BY gender, emp_no % 3 + PI() DESC LIMIT 8;
408+
409+
gender:s |emp_no % 3 + PI():d
410+
------------+------------------
411+
null |5.1415926535
412+
null |4.1415926535
413+
null |3.1415926535
414+
F |5.1415926535
415+
F |4.1415926535
416+
F |3.1415926535
417+
M |5.1415926535
418+
M |4.1415926535
419+
;
420+
421+
groupByFieldAndConstantScalarWithAliasWithOrderByDesc
422+
SELECT gender, emp_no % 3 + PI() as p FROM test_emp GROUP BY gender, emp_no % 3 + PI() ORDER BY gender DESC, p DESC LIMIT 8;
423+
424+
gender:s |p:d
425+
------------+------------------
426+
M |5.1415926535
427+
M |4.1415926535
428+
M |3.1415926535
429+
F |5.1415926535
430+
F |4.1415926535
431+
F |3.1415926535
432+
null |5.1415926535
433+
null |4.1415926535
434+
;
247435

248436
//
249437
// Grouping functions

‎x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec

+4
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ groupByMulScalar
4949
SELECT emp_no * 2 AS e FROM test_emp GROUP BY e ORDER BY e;
5050
groupByModScalar
5151
SELECT (emp_no % 3) + 1 AS e FROM test_emp GROUP BY e ORDER BY e;
52+
groupByCastScalar
53+
SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) FROM test_emp GROUP BY CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) ORDER BY CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) NULLS FIRST;
54+
groupByCastScalarWithAlias
55+
SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) as "cast" FROM test_emp GROUP BY "cast" ORDER BY "cast" NULLS FIRST;
5256

5357
// group by nested functions with no alias
5458
groupByTruncate

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

+8-4
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
import java.util.Map;
6666
import java.util.Objects;
6767
import java.util.Set;
68+
import java.util.stream.Collectors;
6869

6970
import static java.util.Collections.emptyList;
7071
import static java.util.Collections.singletonList;
@@ -625,12 +626,15 @@ protected LogicalPlan rule(LogicalPlan plan) {
625626
.map(or -> tryResolveExpression(or, o.child()))
626627
.collect(toList());
627628

628-
AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream()
629-
.filter(Expression::resolved)
630-
.collect(toList()));
631629

630+
Set<Expression> resolvedRefs = maybeResolved.stream()
631+
.filter(Expression::resolved)
632+
.collect(Collectors.toSet());
632633

633-
AttributeSet missing = resolvedRefs.subtract(o.child().outputSet());
634+
AttributeSet missing = Expressions.filterReferences(
635+
resolvedRefs,
636+
o.child().outputSet()
637+
);
634638

635639
if (!missing.isEmpty()) {
636640
// Add missing attributes but project them away afterwards

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ protected VerificationException(Collection<Failure> sources) {
2727
public String getMessage() {
2828
return failures.stream()
2929
.map(f -> {
30-
Location l = f.source().source().source();
30+
Location l = f.node().source().source();
3131
return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message();
3232
})
3333
.collect(Collectors.joining(StringUtils.NEW_LINE, "Found " + failures.size() + " problem(s)\n", StringUtils.EMPTY));

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+8-8
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,16 @@ public Verifier(Metrics metrics) {
7777
}
7878

7979
static class Failure {
80-
private final Node<?> source;
80+
private final Node<?> node;
8181
private final String message;
8282

83-
Failure(Node<?> source, String message) {
84-
this.source = source;
83+
Failure(Node<?> node, String message) {
84+
this.node = node;
8585
this.message = message;
8686
}
8787

88-
Node<?> source() {
89-
return source;
88+
Node<?> node() {
89+
return node;
9090
}
9191

9292
String message() {
@@ -95,7 +95,7 @@ String message() {
9595

9696
@Override
9797
public int hashCode() {
98-
return source.hashCode();
98+
return Objects.hash(node);
9999
}
100100

101101
@Override
@@ -109,7 +109,7 @@ public boolean equals(Object obj) {
109109
}
110110

111111
Verifier.Failure other = (Verifier.Failure) obj;
112-
return Objects.equals(source, other.source);
112+
return Objects.equals(node, other.node);
113113
}
114114

115115
@Override
@@ -124,7 +124,7 @@ private static Failure fail(Node<?> source, String message, Object... args) {
124124

125125
public Map<Node<?>, String> verifyFailures(LogicalPlan plan) {
126126
Collection<Failure> failures = verify(plan);
127-
return failures.stream().collect(toMap(Failure::source, Failure::message));
127+
return failures.stream().collect(toMap(Failure::node, Failure::message));
128128
}
129129

130130
Collection<Failure> verify(LogicalPlan plan) {

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,14 @@ static class AttributeWrapper {
3232

3333
@Override
3434
public int hashCode() {
35-
return attr.semanticHash();
35+
return attr.hashCode();
3636
}
3737

3838
@Override
3939
public boolean equals(Object obj) {
4040
if (obj instanceof AttributeWrapper) {
4141
AttributeWrapper aw = (AttributeWrapper) obj;
42-
return attr.semanticEquals(aw.attr);
42+
return attr.equals(aw.attr);
4343
}
4444

4545
return false;
@@ -368,4 +368,4 @@ public boolean equals(Object obj) {
368368
public String toString() {
369369
return delegate.toString();
370370
}
371-
}
371+
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java

-3
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,6 @@ public boolean resolved() {
126126

127127
public abstract DataType dataType();
128128

129-
@Override
130-
public abstract int hashCode();
131-
132129
@Override
133130
public String toString() {
134131
return nodeName() + "[" + propertiesToString(false) + "]";

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java

+28
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,11 @@
1111

1212
import java.util.ArrayList;
1313
import java.util.Collection;
14+
import java.util.LinkedHashSet;
1415
import java.util.List;
16+
import java.util.Set;
1517
import java.util.function.Predicate;
18+
import java.util.stream.Collectors;
1619

1720
import static java.util.Collections.emptyList;
1821
import static java.util.Collections.emptyMap;
@@ -99,6 +102,31 @@ public static AttributeSet references(List<? extends Expression> exps) {
99102
return set;
100103
}
101104

105+
public static AttributeSet filterReferences(Set<? extends Expression> exps, AttributeSet excluded) {
106+
AttributeSet ret = new AttributeSet();
107+
while (exps.size() > 0) {
108+
109+
Set<Expression> filteredExps = new LinkedHashSet<>();
110+
for (Expression exp : exps) {
111+
Expression attr = Expressions.attribute(exp);
112+
if (attr == null || (excluded.contains(attr) == false)) {
113+
filteredExps.add(exp);
114+
}
115+
}
116+
117+
ret.addAll(new AttributeSet(
118+
filteredExps.stream().filter(c->c.children().isEmpty())
119+
.flatMap(exp->exp.references().stream())
120+
.collect(Collectors.toSet())
121+
));
122+
123+
exps = filteredExps.stream()
124+
.flatMap((Expression exp)->exp.children().stream())
125+
.collect(Collectors.toSet());
126+
}
127+
return ret;
128+
}
129+
102130
public static String name(Expression e) {
103131
return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName();
104132
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java

-5
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,6 @@ private FieldAttribute innerField(EsField type) {
9797
return new FieldAttribute(source(), this, name() + "." + type.getName(), type, qualifier(), nullable(), id(), synthetic());
9898
}
9999

100-
@Override
101-
protected Expression canonicalize() {
102-
return new FieldAttribute(source(), null, "<none>", field, null, Nullability.TRUE, id(), false);
103-
}
104-
105100
@Override
106101
protected Attribute clone(Source source, String name, String qualifier, Nullability nullability,
107102
ExpressionId id, boolean synthetic) {

0 commit comments

Comments
 (0)