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)
elastic · Oct 31, 2019 · a759375 · a759375
1 parent 5dc1761
commit a759375
Show file tree

Hide file tree

Showing 25 changed files with 627 additions and 121 deletions.
diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec
@@ -244,6 +244,194 @@ TRUNCATE(YEAR("birth_date"), -2)
 null
 1900
 ;
+// Fails for H2
+groupByCastScalarWithNumericRef
+SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST;
+
+CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT):l
+------------------------------------------------------
+null
+1952
+1953
+1954
+1955
+1956
+1957
+1958
+1959
+1960
+1961
+1962
+1963
+1964
+1965
+;
+
+groupByConvertScalar
+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;
+
+
+CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l
+-----------------------------------------------------------
+null
+1952
+1953
+1954
+1955
+1956
+1957
+1958
+1959
+1960
+1961
+1962
+1963
+1964
+1965
+;
+
+
+groupByConvertScalarWithAlias
+SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) as "convert" FROM test_emp GROUP BY "convert" ORDER BY "convert" NULLS FIRST;
+
+convert:l
+---------
+null
+1952
+1953
+1954
+1955
+1956
+1957
+1958
+1959
+1960
+1961
+1962
+1963
+1964
+1965
+;
+
+groupByConvertScalarWithNumericRef
+SELECT CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT) FROM test_emp GROUP BY 1 ORDER BY 1 NULLS FIRST;
+
+CONVERT(ABS(EXTRACT(YEAR FROM "birth_date")), SQL_BIGINT):l
+-----------------------------------------------------------
+null
+1952
+1953
+1954
+1955
+1956
+1957
+1958
+1959
+1960
+1961
+1962
+1963
+1964
+1965
+;
+
+groupByConstantScalar
+SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no LIMIT 10;
+
+PI() * emp_no:d
+---------------
+31419.06812855152
+31422.209721205112
+31425.3513138587
+31428.49290651229
+31431.63449916588
+31434.77609181947
+31437.91768447306
+31441.05927712665
+31444.200869780238
+31447.34246243383
+;
+
+groupByConstantScalarWithOrderByDesc
+SELECT PI() * emp_no FROM test_emp GROUP BY PI() * emp_no ORDER BY PI() * emp_no DESC LIMIT 10;
+
+PI() * emp_no:d
+-------
+31730.08580125691
+31726.94420860332
+31723.80261594973
+31720.66102329614
+31717.51943064255
+31714.377837988963
+31711.23624533537
+31708.094652681782
+31704.95306002819
+31701.811467374602
+;
+
+groupByConstantScalarWithAlias
+SELECT PI() * emp_no AS "value" FROM test_emp GROUP BY value ORDER BY value LIMIT 10;
+
+value:d
+------------------
+31419.06812855152
+31422.209721205112
+31425.3513138587
+31428.49290651229
+31431.63449916588
+31434.77609181947
+31437.91768447306
+31441.05927712665
+31444.200869780238
+31447.34246243383
+;
+
+groupByConstantScalarWithNumericRef
+SELECT PI() * emp_no FROM test_emp GROUP BY 1 ORDER BY 1 DESC LIMIT 10;
+
+PI() * emp_no:d
+------------------
+31730.08580125691
+31726.94420860332
+31723.80261594973
+31720.66102329614
+31717.51943064255
+31714.377837988963
+31711.23624533537
+31708.094652681782
+31704.95306002819
+31701.811467374602
+;
+
+groupByFieldAndConstantScalarWithMultipleOrderBy
+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;
+
+gender:s    |emp_no % 3 + PI():d
+------------+------------------
+null        |5.141592653589793
+null        |4.141592653589793
+null        |3.141592653589793
+F           |5.141592653589793
+F           |4.141592653589793
+F           |3.141592653589793
+M           |5.141592653589793
+M           |4.141592653589793
+;
+
+groupByFieldAndConstantScalarWithAliasWithOrderByDesc
+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;
+
+gender:s    |p:d
+------------+------------------
+M           |5.141592653589793
+M           |4.141592653589793
+M           |3.141592653589793
+F           |5.141592653589793
+F           |4.141592653589793
+F           |3.141592653589793
+null        |5.141592653589793
+null        |4.141592653589793
+;
 
 //
 // Grouping functions

diff --git a/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec
@@ -49,6 +49,10 @@ groupByMulScalar
 SELECT emp_no * 2 AS e FROM test_emp GROUP BY e ORDER BY e;
 groupByModScalar
 SELECT (emp_no % 3) + 1 AS e FROM test_emp GROUP BY e ORDER BY e;
+groupByCastScalar
+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;
+groupByCastScalarWithAlias
+SELECT CAST(ABS(EXTRACT(YEAR FROM "birth_date")) AS BIGINT) as "cast" FROM test_emp GROUP BY "cast" ORDER BY "cast" NULLS FIRST;
 
 // group by nested functions with no alias
 groupByTruncate

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
@@ -65,6 +65,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
@@ -624,12 +625,15 @@ protected LogicalPlan rule(LogicalPlan plan) {
                         .map(or -> tryResolveExpression(or, o.child()))
                         .collect(toList());
 
-                AttributeSet resolvedRefs = Expressions.references(maybeResolved.stream()
-                        .filter(Expression::resolved)
-                        .collect(toList()));
 
+                Set<Expression> resolvedRefs = maybeResolved.stream()
+                    .filter(Expression::resolved)
+                    .collect(Collectors.toSet());
 
-                AttributeSet missing = resolvedRefs.subtract(o.child().outputSet());
+                AttributeSet missing = Expressions.filterReferences(
+                    resolvedRefs,
+                    o.child().outputSet()
+                );
 
                 if (!missing.isEmpty()) {
                     // Add missing attributes but project them away afterwards

diff --git a/...ql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java b/...ql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerificationException.java
@@ -27,7 +27,7 @@ protected VerificationException(Collection<Failure> sources) {
     public String getMessage() {
         return failures.stream()
                 .map(f -> {
-                    Location l = f.source().source().source();
+                    Location l = f.node().source().source();
                     return "line " + l.getLineNumber() + ":" + l.getColumnNumber() + ": " + f.message();
                 })
                 .collect(Collectors.joining(StringUtils.NEW_LINE, "Found " + failures.size() + " problem(s)\n", StringUtils.EMPTY));

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
@@ -78,16 +78,16 @@ public Verifier(Metrics metrics) {
     }
 
     static class Failure {
-        private final Node<?> source;
+        private final Node<?> node;
         private final String message;
 
-        Failure(Node<?> source, String message) {
-            this.source = source;
+        Failure(Node<?> node, String message) {
+            this.node = node;
             this.message = message;
         }
 
-        Node<?> source() {
-            return source;
+        Node<?> node() {
+            return node;
         }
 
         String message() {
@@ -96,7 +96,7 @@ String message() {
 
         @Override
         public int hashCode() {
-            return source.hashCode();
+            return Objects.hash(node);
         }
 
         @Override
@@ -110,7 +110,7 @@ public boolean equals(Object obj) {
             }
 
             Verifier.Failure other = (Verifier.Failure) obj;
-            return Objects.equals(source, other.source);
+            return Objects.equals(node, other.node);
         }
 
         @Override
@@ -125,7 +125,7 @@ private static Failure fail(Node<?> source, String message, Object... args) {
 
     public Map<Node<?>, String> verifyFailures(LogicalPlan plan) {
         Collection<Failure> failures = verify(plan);
-        return failures.stream().collect(toMap(Failure::source, Failure::message));
+        return failures.stream().collect(toMap(Failure::node, Failure::message));
     }
 
     Collection<Failure> verify(LogicalPlan plan) {

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/AttributeMap.java
@@ -32,14 +32,14 @@ static class AttributeWrapper {
 
         @Override
         public int hashCode() {
-            return attr.semanticHash();
+            return attr.hashCode();
         }
 
         @Override
         public boolean equals(Object obj) {
             if (obj instanceof AttributeWrapper) {
                 AttributeWrapper aw = (AttributeWrapper) obj;
-                return attr.semanticEquals(aw.attr);
+                return attr.equals(aw.attr);
             }
 
             return false;
@@ -368,4 +368,4 @@ public boolean equals(Object obj) {
     public String toString() {
         return delegate.toString();
     }
-}
+}
diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java
@@ -126,9 +126,6 @@ public boolean resolved() {
 
     public abstract DataType dataType();
 
-    @Override
-    public abstract int hashCode();
-
     @Override
     public String toString() {
         return nodeName() + "[" + propertiesToString(false) + "]";

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java
@@ -11,8 +11,11 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
@@ -99,6 +102,31 @@ public static AttributeSet references(List<? extends Expression> exps) {
         return set;
     }
 
+    public static AttributeSet filterReferences(Set<? extends Expression> exps, AttributeSet excluded) {
+        AttributeSet ret = new AttributeSet();
+        while (exps.size() > 0) {
+
+            Set<Expression> filteredExps = new LinkedHashSet<>();
+            for (Expression exp : exps) {
+                Expression attr = Expressions.attribute(exp);
+                if (attr == null || (excluded.contains(attr) == false)) {
+                    filteredExps.add(exp);
+                }
+            }
+
+            ret.addAll(new AttributeSet(
+                filteredExps.stream().filter(c->c.children().isEmpty())
+                .flatMap(exp->exp.references().stream())
+                .collect(Collectors.toSet())
+            ));
+
+            exps = filteredExps.stream()
+                .flatMap((Expression exp)->exp.children().stream())
+                .collect(Collectors.toSet());
+        }
+        return ret;
+    }
+
     public static String name(Expression e) {
         return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName();
     }

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/FieldAttribute.java
@@ -97,11 +97,6 @@ private FieldAttribute innerField(EsField type) {
         return new FieldAttribute(source(), this, name() + "." + type.getName(), type, qualifier(), nullable(), id(), synthetic());
     }
 
-    @Override
-    protected Expression canonicalize() {
-        return new FieldAttribute(source(), null, "<none>", field, null, Nullability.TRUE, id(), false);
-    }
-
     @Override
     protected Attribute clone(Source source, String name, String qualifier, Nullability nullable, ExpressionId id, boolean synthetic) {
         FieldAttribute qualifiedParent = parent != null ? (FieldAttribute) parent.withQualifier(qualifier) : null;