SQL: selecting a literal from grouped by query generates error

[codebird]

related to #41951 and #41413

[astefan]

@codebird thank you for raising the PR. We'll be looking at it, but there may be some time until then. There are several issues in flight related to literals in different scenarios and definitely this one is similar.

[elasticmachine]

Pinging @elastic/es-search

[codebird]

updates, now it resolves both #41413 #41951, but this #34583, still has issues.

[codebird]

@astefan now all 3 issues are solved. 35483 is a bit hacky solution. I'll keep on looking into it... But well if you guys check it before I find anything...

[codebird]

@astefan is this pull request dead? Shall I close it?

[matriv]

Could you please add some unit tests (QueryTranslatorTests) and some integration tests (agg.sql-spec) ?

[matriv]

Left a couple of comments

[codebird]

Don't know what I was thinking to write integrity at the place of integration in my commit message

[astefan]

This LGTM. Thank you for doing all of this and being so patient.
I hate to be the "party" breaker here, but I would like to see one more test, that @matriv also suggested in one of his reviews, but from what I can see, it's missing: #41964 (comment)

I tested the following query and it works, but I would also like to have it in the tests: SELECT 144 / 12 AS division, 1, 2 AS x, 1 + 2 FROM test_emp GROUP BY gender, salary.

Thanks again.

[codebird]

@astefan the test fails because of the column name of 1 + 2 H2 names it 3, ES names it 1 + 2 that's why I added an alias to the addition. Shall we open another issue concerning this, or this is acceptable?

[astefan]

@codebird an alias is fine, thank you. No need for another issue, as I believe we are doing the right thing by keeping the column name as 1 + 2.

[matriv]

@elasticmachine run elasticsearch-ci/2

[matriv]

@codebird I'd like to ask you for one more change please.
After some further investigation we came up with a bit different approach on the fix regarding
the resolution of attributes. The logic is the same though as in your fix: try to resolve groupings and/or aggregates based on the child plan (for example when there is a WHERE clause and the child plan is a filter). So instead of this code: https://github.com/elastic/elasticsearch/pull/41964/files#diff-bb55908282831d2f432f4a4650d55521R345
could you please add the following:

@@ -733,6 +720,19 @@ public class Analyzer extends RuleExecutor<LogicalPlan> {
                 }
             }

+            // Try to resolve aggregates and groupings based on the child plan
+            if (plan instanceof Aggregate) {
+                Aggregate a = (Aggregate) plan;
+                LogicalPlan child = a.child();
+                List<Expression> newGroupings = new ArrayList<>(a.groupings().size());
+                a.groupings().forEach(e -> newGroupings.add(tryResolveExpression(e, child)));
+                List<NamedExpression> newAggregates = new ArrayList<>(a.aggregates().size());
+                a.aggregates().forEach(e -> newAggregates.add(tryResolveExpression(e, child)));
+                if (newAggregates.equals(a.aggregates()) == false || newGroupings.equals(a.groupings()) == false) {
+                    return new Aggregate(a.source(), child, newGroupings, newAggregates);
+                }
+            }
+
             return plan;
         }

[codebird]

much more elegant indeed.

[matriv]

Thanks a lot once again @codebird for your patience and your overall effort on this contribution!

[codebird]

Np.thanks for the good mentoring @matriv