SQL: Fix groupings on empty results and HAVING on local relations #74809

Luegg · 2021-07-01T08:11:18Z

Resolves #74311

The goal of this PR is to make implicit and explicit groupings behave consistently on empty result sets no matter whether the query is run on ES or optimized to a local relation. A typical example is a query like SELECT COUNT(*) FROM test_emp WHERE languages = 2 vs SELECT COUNT(*) FROM test_emp WHERE 1 = 2 where the later used to produce an empty result set instead of the expected 1 row with a count of 0.

The PR also fixes other edge case around contradictions in HAVING clauses that used to produce a wrong result: SELECT COUNT(*) c, 'a' a HAVING COUNT(*) > 1000

Previously:

       c       |       a       
---------------+---------------
1              |a

Now:

       c       |       a       
---------------+---------------

elasticmachine · 2021-07-01T08:11:21Z

Pinging @elastic/es-ql (Team:QL)

cleanups

costin

Looks good overall. Since some rules were refactored, could you please annotate the PR with some comments on what moved where and why? Thanks!

costin · 2021-07-01T17:00:11Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

                );

        Batch refs = new Batch("Replace References", Limiter.ONCE,
                new ReplaceReferenceAttributeWithSource()
                );

+        Batch pushDownFilters = new Batch("Push down filters",
+                // must happen in a batch before ReplaceAggregationsOnLocalRelations
+                new PushDownAndCombineFilters());


Why move this rule into a separate batch instead of moving if before ReplaceAggregationsOnLocalRelationships? The advantage of rules is that two separated filters by themselves could be combined after applying other optimization rules.

I now managed to fix the orders of the rules such that PushDownAndCombineFilters can remain in the original batch.

My problem is that ReplaceAggregationsInLocalRelations, ConstantFolding and PruneFilters form a series of transformations that must be applied in this sequence before PushDownAndCombineFilters. If PushDownAndCombineFilters is applied in the middle of this sequence, it pushes down filters it shouldn't. E.g. for the query SELECT COUNT(*) HAVING COUNT(*) = 0:

Correct sequence:

Having[count(*) = 0] \ Agg[count(*)] \ Local[single]

=Fold=>

Having[false] \ Agg[1] \ Local[single]

=Prune=>

Local[empty]

Incorrect sequence:

Having[count(*) = 0] \ Agg[count(*)] \ Local[single]

=Fold=>

Having[false] \ Agg[1] \ Local[single]

=PushDownFilter=> // pushes down because Having does not contain an aggregation anymore

Agg[1] \ Having[false] \ Local[single]

=Prune=>

Agg[1] \ Local[empty]

It's like ReplaceAggregationsInLocalRelations removes the information that we're dealing with an aggregation and the correct context is only restored with PruneFilter.

costin · 2021-07-01T18:27:12Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+
+                    return plan.transformExpressionsDown(AggregateFunction.class, aggregateFunction -> {
+                        if (aggregateFunction instanceof Count) {
+                            return new Literal(Source.EMPTY, count, DataTypes.LONG);


Copy the aggregateFunction source and datatype instead of Source.EMPTY/Long - it's the same thing but conveys the intent better.

costin · 2021-07-01T18:29:24Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+                        if (aggregateFunction instanceof Count) {
+                            return new Literal(Source.EMPTY, count, DataTypes.LONG);
+                        } else if (count == 0) {
+                            return new Literal(Source.EMPTY, null, aggregateFunction.dataType());


Likely this needs to be DataTypes.NULL as well.

Using the NULL type failed in SQL spec because H2 also returned the original type. I'm using Literal.of now which also preserves the original data type.

costin · 2021-07-01T18:33:04Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+                // This optimization is only correct if `plan` has a LocalRelation descendant that is not filtered.
+                // Because PushDownAndCombineFilters might push down filters to become descendants of `plan`, this optimization
+                // must never run before PushDownAndCombineFilters.
+                LocalRelation relation = unfilteredLocalRelation(plan.child());


Tight relationships between rules are typically an indicator a rule does too much. Here the count evaluation only applies in case there are no filters since they potentially affect the count.
Aren't filters folded for local relationships anyway? If that's the case this rule can only expect to be applied when the child is always a LocalRelation.

See #74809 (comment)

costin · 2021-07-01T18:34:41Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-        protected LogicalPlan rule(Aggregate a) {
-            boolean hasLocalRelation = a.anyMatch(LocalRelation.class::isInstance);
+        protected LogicalPlan rule(UnaryPlan plan) {
+            if (plan instanceof Aggregate || plan instanceof Filter) {


What about OrderBy?

👍 I forgot about order by. I think it will always be pruned in earlier optimizations for the relevant queries but will add it anyway.

costin · 2021-07-01T18:41:48Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-
-                    if (isNonSkippedLocalRelation || onlyConstantAggregations) {
-                        foldedValues = folded;
+                if(relation.executable() instanceof SingletonExecutable) {


Bad formatting.

costin · 2021-07-01T18:43:21Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-                        foldedValues = folded;
+                if(relation.executable() instanceof SingletonExecutable) {
+                    if(plan instanceof Project){
+                        foldedValues = extractConstants(((Project) plan).projections());


extractConstants -> extractLiterals

costin · 2021-07-01T18:43:59Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

                }
            }
            return values;
        }

    }

+    static class SkipQueryForOnlyConstantAggregations extends OptimizerRule<Aggregate> {


Constant -> Literal

costin · 2021-07-01T18:50:59Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+            if(e instanceof Alias){
+                return foldable(((Alias) e).child());
+            } else {
+                return e.foldable();
+            }


This code is familiar - I wonder if it's somewhere else (could be moved into Expressions).
Small nit, if you make it a one liner you can put it directly in the if statement.

Suggested change

if(e instanceof Alias){

return foldable(((Alias) e).child());

} else {

return e.foldable();

}

return (e instanceof Alias ? foldable((Alias) e).child()) : e.foldable());

Yes, there are a few places where we do the alias resolution manually but the methods that are called in the end are always a bit different. So if we would put it into Expression it would need to be fairly generic. Something like T onResolved(Function<Expression, T> fn) that could be used like e.onResolved(Expression::foldable).

Alternatively, it might be possible to resolve the aliases in fold and foldable.

costin · 2021-07-01T18:52:45Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

@@ -195,8 +195,8 @@ public void testLocalExecWithCountAndWhereFalseFilter() {
        PhysicalPlan p = plan("SELECT COUNT(10), COUNT(DISTINCT 20) WHERE 1 = 2" + randomOrderByAndLimit(2));
        assertEquals(LocalExec.class, p.getClass());
        LocalExec le = (LocalExec) p;
-        assertEquals(EmptyExecutable.class, le.executable().getClass());
-        EmptyExecutable ee = (EmptyExecutable) le.executable();
+        assertEquals(SingletonExecutable.class, le.executable().getClass());


Why did the empty executable was replaced by singleton? Was the initial test wrong?

yes, that's one of the cases where SQL engines return one row with an "empty" result. E.g. Postgres:

lukas=# SELECT COUNT(10), COUNT(DISTINCT 20) WHERE 1 = 2; count | count -------+------- 0 | 0 (1 row)

Luegg · 2021-07-09T06:51:28Z

Thanks Costin, I've reverted everything and the PR is now ready for another round of reviews. All other comments from the first reviews are addressed and otherwise there were no changes.

bpintea

LGTM.

bpintea · 2021-07-13T09:19:15Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-                new SkipQueryIfFoldingProjection()
+                new SkipQueryForOnlyLiteralAggregations(),
+                new PushProjectionsIntoLocalRelation(),
+                // must run after `PushProjectionsIntoLocalRelation` because it removes the the distinction between implicit


Suggested change

// must run after `PushProjectionsIntoLocalRelation` because it removes the the distinction between implicit

// must run after `PushProjectionsIntoLocalRelation` because it removes the distinction between implicit

bpintea · 2021-07-13T09:28:48Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-                    if (isNonSkippedLocalRelation || onlyConstantAggregations) {
-                        foldedValues = folded;
+                if (relation.executable() instanceof SingletonExecutable) {
+                    if (plan instanceof Project) {


Personal pref: it'd be ever so slightly more legible if you swapped the branching to match the if (plan instanceof Aggregate) in the else if below.

+1 to Bogdan's comment - having the same condition (check for Aggregate) makes it easier to 'parse' the two branches.

bpintea · 2021-07-14T16:20:52Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+            if (plan instanceof Aggregate || plan instanceof Filter || plan instanceof OrderBy) {
+                LocalRelation relation = unfilteredLocalRelation(plan.child());
+                if (relation != null) {
+                    long count = (relation.executable() instanceof EmptyExecutable ? 0L : 1L);


Nit: parentheses aren't necessary.

costin

LGTM - thanks for iterating on this!

costin · 2021-07-30T07:39:48Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -177,8 +178,13 @@ public LogicalPlan optimize(LogicalPlan verified) {

        Batch local = new Batch("Skip Elasticsearch",
                new SkipQueryOnLimitZero(),
-                new SkipQueryIfFoldingProjection()
+                new SkipQueryForOnlyLiteralAggregations(),


Remove Only - SkipQueryForLiteralAggregrations.

costin · 2021-07-30T07:46:02Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

-                    if (isNonSkippedLocalRelation || onlyConstantAggregations) {
-                        foldedValues = folded;
+                if (relation.executable() instanceof SingletonExecutable) {
+                    if (plan instanceof Project) {


+1 to Bogdan's comment - having the same condition (check for Aggregate) makes it easier to 'parse' the two branches.

costin · 2021-07-30T07:51:42Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+            return plan;
+        }
+
+        private boolean foldable(Expression e) {


Use the ternary operator : return e instanceof Alias ? foldable(((Alias) e).child()) : e.foldable()
Additionally since there's no double aliasing, after unwrapping an alias, return the foldability of the child:

if (e instanceof Alias) { e = ((Alias) e).child() } return e.foldable()

astefan

LGTM

…astic#74809) Resolves elastic#74311 The goal of this PR is to make implicit and explicit groupings behave consistently on empty result sets no matter whether the query is run on ES or optimized to a local relation. A typical example is a query like `SELECT COUNT(*) FROM test_emp WHERE languages = 2` vs `SELECT COUNT(*) FROM test_emp WHERE 1 = 2` where the later used to produce an empty result set instead of the expected 1 row with a count of 0. The PR also fixes other edge case around contradictions in `HAVING` clauses that used to produce a wrong result: `SELECT COUNT(*) c, 'a' a HAVING COUNT(*) > 1000`

elasticsearchmachine · 2021-07-30T13:29:44Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

…4809) (#75889) Resolves #74311 The goal of this PR is to make implicit and explicit groupings behave consistently on empty result sets no matter whether the query is run on ES or optimized to a local relation. A typical example is a query like `SELECT COUNT(*) FROM test_emp WHERE languages = 2` vs `SELECT COUNT(*) FROM test_emp WHERE 1 = 2` where the later used to produce an empty result set instead of the expected 1 row with a count of 0. The PR also fixes other edge case around contradictions in `HAVING` clauses that used to produce a wrong result: `SELECT COUNT(*) c, 'a' a HAVING COUNT(*) > 1000` Co-authored-by: Lukas Wegmann <[email protected]>

…4809) (#75890) Resolves #74311 The goal of this PR is to make implicit and explicit groupings behave consistently on empty result sets no matter whether the query is run on ES or optimized to a local relation. A typical example is a query like `SELECT COUNT(*) FROM test_emp WHERE languages = 2` vs `SELECT COUNT(*) FROM test_emp WHERE 1 = 2` where the later used to produce an empty result set instead of the expected 1 row with a count of 0. The PR also fixes other edge case around contradictions in `HAVING` clauses that used to produce a wrong result: `SELECT COUNT(*) c, 'a' a HAVING COUNT(*) > 1000` Co-authored-by: Lukas Wegmann <[email protected]>

test cases

4c583ff

Luegg added >bug :Analytics/SQL SQL querying v8.0.0 v7.14.0 v7.15.0 v7.13.4 labels Jul 1, 2021

Luegg requested review from costin, astefan, bpintea and matriv July 1, 2021 08:11

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jul 1, 2021

Luegg force-pushed the fix/aggregatesWoGroupByAndFoldableFilter branch 2 times, most recently from b86477c to f13ac46 Compare July 1, 2021 08:20

Luegg removed request for costin, astefan, bpintea and matriv July 1, 2021 08:28

Luegg marked this pull request as draft July 1, 2021 08:36

Luegg added 2 commits July 1, 2021 10:54

fix empty implicit groupings

37a1a1f

cleanups

SQL: Fix groupings on empty results and HAVING on local relations

8503aa3

Luegg force-pushed the fix/aggregatesWoGroupByAndFoldableFilter branch from f13ac46 to 8503aa3 Compare July 1, 2021 08:55

Merge origin/master into fix/aggregatesWoGroupByAndFoldableFilter

9178770

Luegg marked this pull request as ready for review July 1, 2021 11:48

Luegg requested review from costin, matriv, astefan and bpintea July 1, 2021 11:48

costin reviewed Jul 1, 2021

View reviewed changes

Merge origin/master into fix/aggregatesWoGroupByAndFoldableFilter

4291271

bpintea approved these changes Jul 14, 2021

View reviewed changes

joegallo added v7.13.5 and removed v7.13.4 labels Jul 19, 2021

costin approved these changes Jul 30, 2021

View reviewed changes

Luegg added 2 commits July 30, 2021 09:58

Merge origin/master into fix/aggregatesWoGroupByAndFoldableFilter

0d7bb7a

address review comments

2ce1bc7

Luegg added v7.14.1 v7.14.0 and removed v7.13.5 v7.14.0 v7.14.1 labels Jul 30, 2021

probakowski added v7.14.1 and removed v7.14.0 labels Jul 30, 2021

astefan approved these changes Jul 30, 2021

View reviewed changes

Luegg added the auto-backport Automatically create backport pull requests when merged label Jul 30, 2021

Luegg merged commit 54bdc09 into elastic:master Jul 30, 2021

Luegg deleted the fix/aggregatesWoGroupByAndFoldableFilter branch July 30, 2021 13:28

This was referenced Jul 30, 2021

[7.14] SQL: Fix groupings on empty results and HAVING on local relations (#74809) #75889

Merged

[7.x] SQL: Fix groupings on empty results and HAVING on local relations (#74809) #75890

Merged

mark-vieira added v8.0.0-alpha1 and removed v8.0.0 labels Aug 4, 2021

Luegg mentioned this pull request Feb 3, 2022

SQL: Allow locally executed queries with count(*) #41531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Fix groupings on empty results and HAVING on local relations #74809

SQL: Fix groupings on empty results and HAVING on local relations #74809

Luegg commented Jul 1, 2021

elasticmachine commented Jul 1, 2021

costin left a comment

costin Jul 1, 2021

Luegg Jul 5, 2021

costin Jul 1, 2021

costin Jul 1, 2021

Luegg Jul 5, 2021

costin Jul 1, 2021

Luegg Jul 5, 2021

costin Jul 1, 2021

Luegg Jul 5, 2021

costin Jul 1, 2021

costin Jul 1, 2021

costin Jul 1, 2021

costin Jul 1, 2021

Luegg Jul 5, 2021

costin Jul 1, 2021

Luegg Jul 5, 2021

Luegg commented Jul 9, 2021

bpintea left a comment

bpintea Jul 13, 2021

bpintea Jul 13, 2021

costin Jul 30, 2021

bpintea Jul 14, 2021

costin left a comment

costin Jul 30, 2021

costin Jul 30, 2021

costin Jul 30, 2021

astefan left a comment

elasticsearchmachine commented Jul 30, 2021

	// must run after `PushProjectionsIntoLocalRelation` because it removes the the distinction between implicit
	// must run after `PushProjectionsIntoLocalRelation` because it removes the distinction between implicit

SQL: Fix groupings on empty results and HAVING on local relations #74809

SQL: Fix groupings on empty results and HAVING on local relations #74809

Conversation

Luegg commented Jul 1, 2021

elasticmachine commented Jul 1, 2021

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg commented Jul 9, 2021

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jul 30, 2021

💚 Backport successful