SQL: Support SQL folding of GROUP BY Constant #84574

luigidellaquila · 2022-03-02T15:54:55Z

Allows to avoid pruning of constant groupings from the query plan, that could lead to too aggressive optimizations, like:

completely removing the grouping, that leads to different query semantics (ie. duplicates)
doing wrong assumptions about the possibility to perform local search (eg. getting results also when the target index is empty)

This PR overlaps with #84510 but it's more general as a fix. Though #84510, on some specific cases, can have a significant performance impact, so it can still be considered.

Fixes #84490
Fixes #74064

elasticmachine · 2022-03-02T15:54:57Z

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

elasticsearchmachine · 2022-03-02T15:55:41Z

Hi @luigidellaquila, I've created a changelog YAML for you.

elasticsearchmachine · 2022-03-02T16:17:33Z

Hi @luigidellaquila, I've updated the changelog YAML for you.

Luegg · 2022-03-02T16:47:53Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

@@ -423,6 +423,8 @@ else if (value instanceof IntervalDayTime
                    else {
                        throw new SqlIllegalArgumentException("Cannot GROUP BY function {}", exp);
                    }
+                } else if (exp instanceof Literal) {
+                    key = new GroupByValue(aggId, QueryTranslator.nameOf(exp));


I'm a bit surprised that this actually works but apparently the terms aggregation just ignores fields that do not exist and creates a null bucket. I'm not sure though wether this might not introduce other bugs. E.g. if QueryTranslator.nameOf(exp) evaluates to an actual field name. Could it be an option to have a GroupByConstant for this case?

GroupByConstant was my first idea as well (I also implemented it with the exact same name at first), but then it ended up being the exact copy of GroupByValue.

The fact that also scalar functions use GroupByValue (passing the function as a script, see the other constructor) is another pointer that convinced me to use it.

Unless there is some practical differentiation, probably it can be avoided (anyway, I don't have a strong opinion on that).

About the clash with field names, I reasoned a bit about that and did some tests.

On a person (name, surname) schema, the following are possible clashes

SELECT 1 as name FROM person GROUP BY name; // the alias is the same as a field name

That is interpreted correctly

SELECT 1 FROM person GROUP BY 'name'; // the value of the literal is the same as a field name

Here the field name in the terms is 'name', so there is no clash with the field name.

Still, I just realised that you could define a field named 'name'(!!!). In this case, with the same query you would get an error:
Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on ['name'] in order to load field data by uninverting the inverted index. Note that this can use significant memory.

So I think I'll have to specialise the behaviour

Other cases that come to my mind are fields named null, true or false (I think ES does also not stop you from doing this).

Luegg · 2022-03-02T16:50:23Z

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

@@ -279,7 +279,8 @@ protected LogicalPlan rule(Aggregate agg) {
                }
            }

-            if (prunedGroupings.size() > 0) {
+            // avoid complete removal of the grouping, that could change the query semantics


Suggested change

// avoid complete removal of the grouping, that could change the query semantics

// avoid complete removal of the grouping because it would turn the aggregation into an implicit grouping and change the semantics

I think the wording can be a bit stronger here because, as far as I understand, it always changes the semantics.

It does not always change the semantics actually. If there are aggregate projections (ie. implicit groupings), the semantics do not change

In that case there wouldn't be any groupings to remove though?

In that case the explicit groupings (constants), if present, can be safely removed.

astefan

I don't think this approach is correct. One reason is the one @Luegg mentioned, the other is that it's an overkill solution (running an aggregation when one is not actually needed).

luigidellaquila · 2022-03-02T18:12:25Z

it's an overkill solution (running an aggregation when one is not actually needed).

The alternative is something like #84510, but:

alone it doesn't cover all the cases
this kind of optimisations is extremely error prone

Probably the good solution is a mix of the two, ie. support the constant aggregation properly, addressing the issue pointed out by @Luegg, AND reduce the cost of the operation with a LIMIT 1.

Any further feedback is welcome

Luegg · 2022-03-03T08:21:34Z

Did you also explore solutions around making the behavior in the optimizer and query folder analog to what's happening for implicit aggregations and only differentiate the behavior during query execution (see Querier)? ~~We already have the query logic for the implicit aggregation in place and it should be equivalent for the group by constant case.~~

Update: I just realized that this leaves out possible optimizations...

…l_folder' into group_by_constant_in_sql_folder

luigidellaquila · 2022-03-04T16:55:54Z

Added some improvements to the solution.
As suggested by @Luegg, the approach is very similar to implicit aggregations, but it still needed some specialisation.

luigidellaquila · 2022-03-04T17:41:19Z

@elasticmachine update branch

…l_folder' into group_by_constant_in_sql_folder

elasticsearchmachine · 2024-02-14T18:10:52Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

Support SQL folding of GROUP BY Constant

1fe7615

luigidellaquila added >bug :Analytics/SQL SQL querying Team:QL (Deprecated) Meta label for query languages team labels Mar 2, 2022

luigidellaquila requested review from costin, astefan, Luegg and bpintea March 2, 2022 15:54

elasticsearchmachine added the v8.2.0 label Mar 2, 2022

Update docs/changelog/84574.yaml

27cb926

luigidellaquila mentioned this pull request Mar 2, 2022

SQL: Fix duplicates on GROUP BY constant (without aggregate projections) #84510

Open

Update docs/changelog/84574.yaml

a115927

Luegg reviewed Mar 2, 2022

View reviewed changes

astefan requested changes Mar 2, 2022

View reviewed changes

luigidellaquila mentioned this pull request Mar 3, 2022

SQL: GROUP BY constant with ORDER BY aggregate crashes the server #84613

Open

luigidellaquila marked this pull request as draft March 3, 2022 13:02

luigidellaquila added 2 commits March 4, 2022 17:46

Refine translation of GROUP BY CONSTANT using global aggregations

8cdcc6c

Merge remote-tracking branch 'luigidellaquila/group_by_constant_in_sq…

1c691f5

…l_folder' into group_by_constant_in_sql_folder

elasticmachine and others added 3 commits March 4, 2022 11:41

Merge branch 'master' into group_by_constant_in_sql_folder

11ddf13

Fix aggregation queries with constant grouping

ba36347

Merge remote-tracking branch 'luigidellaquila/group_by_constant_in_sq…

cea5518

…l_folder' into group_by_constant_in_sql_folder

luigidellaquila marked this pull request as ready for review March 7, 2022 16:16

Merge branch 'master' into group_by_constant_in_sql_folder

4735943

craigtaverner added the v8.4.0 label May 25, 2022

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:08

mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022

csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022

kingherc added v8.7.0 and removed v8.6.0 labels Nov 16, 2022

rjernst added v8.8.0 and removed v8.7.0 labels Feb 8, 2023

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

pugnascotia added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

elasticsearchmachine added v8.14.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed v8.13.0 labels Feb 14, 2024

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

elasticsearchmachine added v8.16.0 and removed v8.15.0 labels Jul 4, 2024

mark-vieira added v9.0.0 and removed v8.16.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Support SQL folding of GROUP BY Constant #84574

SQL: Support SQL folding of GROUP BY Constant #84574

luigidellaquila commented Mar 2, 2022 •

edited

Loading

elasticmachine commented Mar 2, 2022

elasticsearchmachine commented Mar 2, 2022

elasticsearchmachine commented Mar 2, 2022

Luegg Mar 2, 2022

luigidellaquila Mar 2, 2022

Luegg Mar 3, 2022

Luegg Mar 2, 2022

luigidellaquila Mar 2, 2022

Luegg Mar 3, 2022

luigidellaquila Mar 7, 2022

astefan left a comment

luigidellaquila commented Mar 2, 2022

Luegg commented Mar 3, 2022 •

edited

Loading

luigidellaquila commented Mar 4, 2022

luigidellaquila commented Mar 4, 2022

elasticsearchmachine commented Feb 14, 2024

	// avoid complete removal of the grouping, that could change the query semantics
	// avoid complete removal of the grouping because it would turn the aggregation into an implicit grouping and change the semantics

SQL: Support SQL folding of GROUP BY Constant #84574

Are you sure you want to change the base?

SQL: Support SQL folding of GROUP BY Constant #84574

Conversation

luigidellaquila commented Mar 2, 2022 • edited Loading

elasticmachine commented Mar 2, 2022

elasticsearchmachine commented Mar 2, 2022

elasticsearchmachine commented Mar 2, 2022

Luegg Mar 2, 2022

Choose a reason for hiding this comment

luigidellaquila Mar 2, 2022

Choose a reason for hiding this comment

Luegg Mar 3, 2022

Choose a reason for hiding this comment

Luegg Mar 2, 2022

Choose a reason for hiding this comment

luigidellaquila Mar 2, 2022

Choose a reason for hiding this comment

Luegg Mar 3, 2022

Choose a reason for hiding this comment

luigidellaquila Mar 7, 2022

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

luigidellaquila commented Mar 2, 2022

Luegg commented Mar 3, 2022 • edited Loading

luigidellaquila commented Mar 4, 2022

luigidellaquila commented Mar 4, 2022

elasticsearchmachine commented Feb 14, 2024

luigidellaquila commented Mar 2, 2022 •

edited

Loading

Luegg commented Mar 3, 2022 •

edited

Loading