From 2707a44c9f5b5ae1aed79c960c1ecbbfb9db95af Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 30 Jan 2019 11:22:28 +0200 Subject: [PATCH 1/5] Add checks for Grouping functions restriction to be placed inside GROUP BY --- .../xpack/sql/analysis/analyzer/Verifier.java | 32 +++++++++++++++++ .../analyzer/VerifierErrorMessagesTests.java | 35 +++++++++++++++++++ 2 files changed, 67 insertions(+) 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 index 3f363d5a92809..3704d7cdc2655 100644 --- 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 @@ -225,6 +225,7 @@ Collection verify(LogicalPlan plan) { validateInExpression(p, localFailures); validateConditional(p, localFailures); + checkHistogramInGrouping(p, localFailures); checkFilterOnAggs(p, localFailures); checkFilterOnGrouping(p, localFailures); @@ -560,6 +561,37 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { + // check if the query has a grouping function (Histogram) but no GROUP BY + if (p instanceof Project) { + Project proj = (Project) p; + + proj.projections().forEach(e -> { + if (Functions.isGrouping(e) == false) { + e.collectFirstChildren(c -> { + if (Functions.isGrouping(c)) { + localFailures.add(fail(c, "[%s] needs to be part of the grouping", Expressions.name(c))); + return true; + } + return false; + }); + } else { + localFailures.add(fail(e, "[%s] needs to be part of the grouping", Expressions.name(e))); + } + }); + } else if (p instanceof Aggregate) { + // if it does have a GROUP BY, check if the groupings contain the grouping functions (Histograms) + Aggregate a = (Aggregate) p; + + a.aggregates().forEach(as -> { + Expression exp = as instanceof Alias ? ((Alias) as).child() : as; + if (Functions.isGrouping(exp) && false == Expressions.anyMatch(a.groupings(), g -> exp.semanticEquals(g))) { + localFailures.add(fail(exp, "[%s] needs to be part of the grouping", Expressions.name(exp))); + } + }); + } + } private static void checkFilterOnAggs(LogicalPlan p, Set localFailures) { if (p instanceof Filter) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 4279910e0e03b..8b3193de2243c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -536,6 +536,41 @@ public void testAggsInHistogram() { assertEquals("1:47: Cannot use an aggregate [MAX] for grouping", error("SELECT MAX(date) FROM test GROUP BY HISTOGRAM(MAX(int), 1)")); } + + public void testHistogramNotInGrouping() { + assertEquals("1:8: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) AS h FROM test")); + } + + public void testHistogramNotInGroupingWithCount() { + assertEquals("1:8: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) AS h, COUNT(*) FROM test")); + } + + public void testHistogramNotInGroupingWithMaxFirst() { + assertEquals("1:19: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT MAX(date), HISTOGRAM(date, INTERVAL 1 MONTH) AS h FROM test")); + } + + public void testHistogramWithoutAliasNotInGrouping() { + assertEquals("1:8: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) FROM test")); + } + + public void testTwoHistogramsNotInGrouping() { + assertEquals("1:48: [HISTOGRAM(date, INTERVAL 1 DAY)] needs to be part of the grouping", + error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) AS h, HISTOGRAM(date, INTERVAL 1 DAY) FROM test GROUP BY h")); + } + + public void testHistogramNotInGrouping_WithGroupByField() { + assertEquals("1:8: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) FROM test GROUP BY date")); + } + + public void testScalarOfHistogramNotInGrouping() { + assertEquals("1:14: [HISTOGRAM(date, INTERVAL 1 MONTH)] needs to be part of the grouping", + error("SELECT MONTH(HISTOGRAM(date, INTERVAL 1 MONTH)) FROM test")); + } public void testErrorMessageForPercentileWithSecondArgBasedOnAField() { assertEquals("1:8: Second argument of PERCENTILE must be a constant, received [ABS(int)]", From 51b19ef9465d41e12b69cda1bec9a8b97a6cf855 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Thu, 31 Jan 2019 08:48:49 +0200 Subject: [PATCH 2/5] Fixed bug where GROUP BY HISTOGRAM (not using alias) wasn't recognized properly in the Verifier due to functions equality not working correctly. --- .../sql/qa/src/main/resources/agg.csv-spec | 23 +++++++++++++++ .../xpack/sql/analysis/analyzer/Verifier.java | 28 ++++++------------- .../function/grouping/GroupingFunction.java | 27 ++++++++---------- .../function/grouping/Histogram.java | 17 +++++++---- 4 files changed, 54 insertions(+), 41 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 index 1cf3acdcfa4a6..9e8fcba075217 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec @@ -319,6 +319,29 @@ SELECT HISTOGRAM(emp_no % 100, 10) AS h, COUNT(*) as c FROM test_emp GROUP BY h 0 |10 ; +histogramGroupByWithoutAlias +schema::h:ts|c:l +SELECT HISTOGRAM(birth_date, INTERVAL 1 YEAR) AS h, COUNT(*) as c FROM test_emp GROUP BY HISTOGRAM(birth_date, INTERVAL 1 YEAR) ORDER BY h DESC; + + h | c +--------------------+--------------- +1964-02-02T00:00:00Z|5 +1963-02-07T00:00:00Z|7 +1962-02-12T00:00:00Z|6 +1961-02-17T00:00:00Z|8 +1960-02-23T00:00:00Z|7 +1959-02-28T00:00:00Z|9 +1958-03-05T00:00:00Z|6 +1957-03-10T00:00:00Z|6 +1956-03-15T00:00:00Z|4 +1955-03-21T00:00:00Z|4 +1954-03-26T00:00:00Z|7 +1953-03-31T00:00:00Z|10 +1952-04-05T00:00:00Z|10 +1951-04-11T00:00:00Z|1 +null |10 +; + countAll schema::all_names:l|c:l SELECT COUNT(ALL first_name) all_names, COUNT(*) c FROM test_emp; 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 index 3704d7cdc2655..8670d6eee1c30 100644 --- 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 @@ -19,6 +19,7 @@ import org.elasticsearch.xpack.sql.expression.function.Functions; import org.elasticsearch.xpack.sql.expression.function.Score; import org.elasticsearch.xpack.sql.expression.function.aggregate.AggregateFunctionAttribute; +import org.elasticsearch.xpack.sql.expression.function.grouping.GroupingFunction; import org.elasticsearch.xpack.sql.expression.function.grouping.GroupingFunctionAttribute; import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction; import org.elasticsearch.xpack.sql.expression.predicate.conditional.ConditionalFunction; @@ -566,30 +567,17 @@ private static void checkHistogramInGrouping(LogicalPlan p, Set localFa // check if the query has a grouping function (Histogram) but no GROUP BY if (p instanceof Project) { Project proj = (Project) p; - - proj.projections().forEach(e -> { - if (Functions.isGrouping(e) == false) { - e.collectFirstChildren(c -> { - if (Functions.isGrouping(c)) { - localFailures.add(fail(c, "[%s] needs to be part of the grouping", Expressions.name(c))); - return true; - } - return false; - }); - } else { - localFailures.add(fail(e, "[%s] needs to be part of the grouping", Expressions.name(e))); - } - }); + proj.projections().forEach(e -> e.forEachDown(f -> localFailures.add(fail(f, "[%s] needs to be part of the grouping", + Expressions.name(f))), GroupingFunction.class)); } else if (p instanceof Aggregate) { // if it does have a GROUP BY, check if the groupings contain the grouping functions (Histograms) Aggregate a = (Aggregate) p; - - a.aggregates().forEach(as -> { - Expression exp = as instanceof Alias ? ((Alias) as).child() : as; - if (Functions.isGrouping(exp) && false == Expressions.anyMatch(a.groupings(), g -> exp.semanticEquals(g))) { - localFailures.add(fail(exp, "[%s] needs to be part of the grouping", Expressions.name(exp))); + a.aggregates().forEach(agg -> agg.forEachDown(e -> { + if (Expressions.anyMatch(a.groupings(), g -> {return g instanceof Function && e.functionEquals((Function) g);}) == false + || a.groupings().size() == 0) { + localFailures.add(fail(e, "[%s] needs to be part of the grouping", Expressions.name(e))); } - }); + }, GroupingFunction.class)); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java index 0595e29176a93..43280154601cf 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java @@ -26,23 +26,29 @@ public abstract class GroupingFunction extends Function { private final Expression field; + private final List extraChildren; private final List parameters; private GroupingFunctionAttribute lazyAttribute; - protected GroupingFunction(Source source, Expression field) { - this(source, field, emptyList()); + protected GroupingFunction(Source source, Expression field, List extraChildren) { + this(source, field, extraChildren, emptyList()); } - protected GroupingFunction(Source source, Expression field, List parameters) { - super(source, CollectionUtils.combine(singletonList(field), parameters)); + protected GroupingFunction(Source source, Expression field, List extraChildren, List parameters) { + super(source, CollectionUtils.combine(singletonList(field), extraChildren, parameters)); this.field = field; + this.extraChildren = extraChildren == null ? emptyList() : extraChildren; this.parameters = parameters; } public Expression field() { return field; } + + public List extraChildren() { + return extraChildren; + } public List parameters() { return parameters; @@ -57,16 +63,6 @@ public GroupingFunctionAttribute toAttribute() { return lazyAttribute; } - @Override - public final GroupingFunction replaceChildren(List newChildren) { - if (newChildren.size() != 1) { - throw new IllegalArgumentException("expected [1] child but received [" + newChildren.size() + "]"); - } - return replaceChild(newChildren.get(0)); - } - - protected abstract GroupingFunction replaceChild(Expression newChild); - @Override protected Pipe makePipe() { // unresolved AggNameInput (should always get replaced by the folder) @@ -85,11 +81,12 @@ public boolean equals(Object obj) { } GroupingFunction other = (GroupingFunction) obj; return Objects.equals(other.field(), field()) + && Objects.equals(other.extraChildren(), extraChildren()) && Objects.equals(other.parameters(), parameters()); } @Override public int hashCode() { - return Objects.hash(field(), parameters()); + return Objects.hash(field(), extraChildren(), parameters()); } } \ No newline at end of file diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java index 3dd4bdc992cb0..afec5e3e073cf 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java @@ -10,12 +10,14 @@ import org.elasticsearch.xpack.sql.expression.Expressions; import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal; import org.elasticsearch.xpack.sql.expression.Literal; -import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.tree.NodeInfo; +import org.elasticsearch.xpack.sql.tree.Source; import org.elasticsearch.xpack.sql.type.DataType; import org.elasticsearch.xpack.sql.type.DataTypes; import java.time.ZoneId; +import java.util.Arrays; +import java.util.List; import java.util.Objects; public class Histogram extends GroupingFunction { @@ -24,8 +26,8 @@ public class Histogram extends GroupingFunction { private final ZoneId zoneId; public Histogram(Source source, Expression field, Expression interval, ZoneId zoneId) { - super(source, field); - this.interval = (Literal) interval; + super(source, field, Arrays.asList(interval)); + this.interval = Literal.of(interval); this.zoneId = zoneId; } @@ -51,10 +53,13 @@ protected TypeResolution resolveType() { return resolution; } - + @Override - protected GroupingFunction replaceChild(Expression newChild) { - return new Histogram(source(), newChild, interval, zoneId); + public final GroupingFunction replaceChildren(List newChildren) { + if (newChildren.size() != 2) { + throw new IllegalArgumentException("expected [2] children but received [" + newChildren.size() + "]"); + } + return new Histogram(source(), newChildren.get(0), newChildren.get(1), zoneId); } @Override From 2658b70efec82a75fe3311b58497cd3298d97ade Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Sat, 2 Feb 2019 01:20:09 +0200 Subject: [PATCH 3/5] Address comments --- .../xpack/sql/analysis/analyzer/Verifier.java | 8 ++++---- .../function/grouping/GroupingFunction.java | 17 +++++------------ 2 files changed, 9 insertions(+), 16 deletions(-) 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 index de1fe5f15e7c1..8261752e66ef3 100644 --- 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 @@ -591,14 +591,14 @@ private static void checkHistogramInGrouping(LogicalPlan p, Set localFa // check if the query has a grouping function (Histogram) but no GROUP BY if (p instanceof Project) { Project proj = (Project) p; - proj.projections().forEach(e -> e.forEachDown(f -> localFailures.add(fail(f, "[%s] needs to be part of the grouping", - Expressions.name(f))), GroupingFunction.class)); + proj.projections().forEach(e -> e.forEachDown(f -> + localFailures.add(fail(f, "[%s] needs to be part of the grouping", Expressions.name(f))), GroupingFunction.class)); } else if (p instanceof Aggregate) { // if it does have a GROUP BY, check if the groupings contain the grouping functions (Histograms) Aggregate a = (Aggregate) p; a.aggregates().forEach(agg -> agg.forEachDown(e -> { - if (Expressions.anyMatch(a.groupings(), g -> {return g instanceof Function && e.functionEquals((Function) g);}) == false - || a.groupings().size() == 0) { + if (a.groupings().size() == 0 + || Expressions.anyMatch(a.groupings(), g -> g instanceof Function && e.functionEquals((Function) g)) == false) { localFailures.add(fail(e, "[%s] needs to be part of the grouping", Expressions.name(e))); } }, GroupingFunction.class)); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java index 43280154601cf..b8a6bb4054095 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java @@ -26,29 +26,23 @@ public abstract class GroupingFunction extends Function { private final Expression field; - private final List extraChildren; private final List parameters; private GroupingFunctionAttribute lazyAttribute; - protected GroupingFunction(Source source, Expression field, List extraChildren) { - this(source, field, extraChildren, emptyList()); + protected GroupingFunction(Source source, Expression field) { + this(source, field, emptyList()); } - protected GroupingFunction(Source source, Expression field, List extraChildren, List parameters) { - super(source, CollectionUtils.combine(singletonList(field), extraChildren, parameters)); + protected GroupingFunction(Source source, Expression field, List parameters) { + super(source, CollectionUtils.combine(singletonList(field), parameters)); this.field = field; - this.extraChildren = extraChildren == null ? emptyList() : extraChildren; this.parameters = parameters; } public Expression field() { return field; } - - public List extraChildren() { - return extraChildren; - } public List parameters() { return parameters; @@ -81,12 +75,11 @@ public boolean equals(Object obj) { } GroupingFunction other = (GroupingFunction) obj; return Objects.equals(other.field(), field()) - && Objects.equals(other.extraChildren(), extraChildren()) && Objects.equals(other.parameters(), parameters()); } @Override public int hashCode() { - return Objects.hash(field(), extraChildren(), parameters()); + return Objects.hash(field(), parameters()); } } \ No newline at end of file From 08a2bf7cce3868b306d6cd8598240ba1f40af571 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Sat, 2 Feb 2019 03:11:41 +0200 Subject: [PATCH 4/5] Changes after merging master --- .../xpack/sql/analysis/analyzer/Verifier.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 index 77bb547132dee..ed9bd1f106830 100644 --- 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 @@ -230,7 +230,7 @@ Collection verify(LogicalPlan plan) { validateInExpression(p, localFailures); validateConditional(p, localFailures); - checkHistogramInGrouping(p, localFailures); + checkGroupingFunctionInGroupBy(p, localFailures); checkFilterOnAggs(p, localFailures); checkFilterOnGrouping(p, localFailures); @@ -589,19 +589,19 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures) { + private static void checkGroupingFunctionInGroupBy(LogicalPlan p, Set localFailures) { // check if the query has a grouping function (Histogram) but no GROUP BY if (p instanceof Project) { Project proj = (Project) p; proj.projections().forEach(e -> e.forEachDown(f -> - localFailures.add(fail(f, "[%s] needs to be part of the grouping", Expressions.name(f))), GroupingFunction.class)); + localFailures.add(fail(f, "[{}] needs to be part of the grouping", Expressions.name(f))), GroupingFunction.class)); } else if (p instanceof Aggregate) { // if it does have a GROUP BY, check if the groupings contain the grouping functions (Histograms) Aggregate a = (Aggregate) p; a.aggregates().forEach(agg -> agg.forEachDown(e -> { if (a.groupings().size() == 0 || Expressions.anyMatch(a.groupings(), g -> g instanceof Function && e.functionEquals((Function) g)) == false) { - localFailures.add(fail(e, "[%s] needs to be part of the grouping", Expressions.name(e))); + localFailures.add(fail(e, "[{}] needs to be part of the grouping", Expressions.name(e))); } }, GroupingFunction.class)); } From 597eeaa2de0225469caa928f95b6191c73ff28d4 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Sat, 2 Feb 2019 13:08:08 +0200 Subject: [PATCH 5/5] Address comment --- .../xpack/sql/expression/function/grouping/Histogram.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java index afec5e3e073cf..23061bfea1859 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java @@ -16,7 +16,7 @@ import org.elasticsearch.xpack.sql.type.DataTypes; import java.time.ZoneId; -import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Objects; @@ -26,7 +26,7 @@ public class Histogram extends GroupingFunction { private final ZoneId zoneId; public Histogram(Source source, Expression field, Expression interval, ZoneId zoneId) { - super(source, field, Arrays.asList(interval)); + super(source, field, Collections.singletonList(interval)); this.interval = Literal.of(interval); this.zoneId = zoneId; }