Skip to content

Commit 185e067

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 7ea7491 commit 185e067

File tree

26 files changed

+627
-99
lines changed

26 files changed

+627
-99
lines changed

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

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

250438
//
251439
// Grouping functions

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

+4
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ groupByMulScalar
5151
SELECT emp_no * 2 AS e FROM test_emp GROUP BY e ORDER BY e;
5252
groupByModScalar
5353
SELECT (emp_no % 3) + 1 AS e FROM test_emp GROUP BY e ORDER BY e;
54+
groupByCastScalar
55+
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;
56+
groupByCastScalarWithAlias
57+
SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) as "cast" FROM test_emp GROUP BY "cast" ORDER BY "cast" NULLS FIRST;
5458

5559
// group by nested functions with no alias
5660
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
@@ -66,6 +66,7 @@
6666
import java.util.Map;
6767
import java.util.Objects;
6868
import java.util.Set;
69+
import java.util.stream.Collectors;
6970

7071
import static java.util.Collections.emptyList;
7172
import static java.util.Collections.singletonList;
@@ -609,12 +610,15 @@ protected LogicalPlan rule(LogicalPlan plan) {
609610
.map(or -> tryResolveExpression(or, o.child()))
610611
.collect(toList());
611612

612-
AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream()
613-
.filter(Expression::resolved)
614-
.collect(toList()));
615613

614+
Set<Expression> resolvedRefs = maybeResolved.stream()
615+
.filter(Expression::resolved)
616+
.collect(Collectors.toSet());
616617

617-
AttributeSet missing = resolvedRefs.subtract(o.child().outputSet());
618+
AttributeSet missing = Expressions.filterReferences(
619+
resolvedRefs,
620+
o.child().outputSet()
621+
);
618622

619623
if (!missing.isEmpty()) {
620624
// 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
@@ -84,16 +84,16 @@ public Verifier(Metrics metrics) {
8484
}
8585

8686
static class Failure {
87-
private final Node<?> source;
87+
private final Node<?> node;
8888
private final String message;
8989

90-
Failure(Node<?> source, String message) {
91-
this.source = source;
90+
Failure(Node<?> node, String message) {
91+
this.node = node;
9292
this.message = message;
9393
}
9494

95-
Node<?> source() {
96-
return source;
95+
Node<?> node() {
96+
return node;
9797
}
9898

9999
String message() {
@@ -102,7 +102,7 @@ String message() {
102102

103103
@Override
104104
public int hashCode() {
105-
return source.hashCode();
105+
return Objects.hash(node);
106106
}
107107

108108
@Override
@@ -116,7 +116,7 @@ public boolean equals(Object obj) {
116116
}
117117

118118
Verifier.Failure other = (Verifier.Failure) obj;
119-
return Objects.equals(source, other.source);
119+
return Objects.equals(node, other.node);
120120
}
121121

122122
@Override
@@ -131,7 +131,7 @@ private static Failure fail(Node<?> source, String message, Object... args) {
131131

132132
public Map<Node<?>, String> verifyFailures(LogicalPlan plan) {
133133
Collection<Failure> failures = verify(plan);
134-
return failures.stream().collect(toMap(Failure::source, Failure::message));
134+
return failures.stream().collect(toMap(Failure::node, Failure::message));
135135
}
136136

137137
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

+26
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.util.List;
1717
import java.util.Set;
1818
import java.util.function.Predicate;
19+
import java.util.stream.Collectors;
1920

2021
import static java.util.Collections.emptyList;
2122
import static java.util.Collections.emptyMap;
@@ -102,6 +103,31 @@ public static AttributeSet references(List<? extends Expression> exps) {
102103
return set;
103104
}
104105

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

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

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

105-
@Override
106-
protected Expression canonicalize() {
107-
return new FieldAttribute(source(), null, "<none>", field, null, Nullability.TRUE, id(), false);
108-
}
109-
110105
@Override
111106
protected Attribute clone(Source source, String name, DataType type, String qualifier,
112107
Nullability nullability, ExpressionId id, boolean synthetic) {

0 commit comments

Comments
 (0)