Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ESQL: Fix layout when aggregating with aliases #117832

Merged
merged 3 commits into from
Dec 3, 2024
Merged

ESQL: Fix layout when aggregating with aliases #117832

merged 3 commits into from
Dec 3, 2024

Conversation

alex-spies
Copy link
Contributor

@alex-spies alex-spies commented Dec 2, 2024 •
edited
Loading

Fix #117770
Fix #117784

#117699 made changes to how we plan aggregations which were supposed to only trigger when a query contained a CATEGORIZE, but accidentally changed a code path that seems to only be required for interoperability with pre-8.13 nodes. Because of this, we didn't notice failing tests until the periodic bwc tests ran.

The code this PR fixes addresses situations where Aggregate plan nodes contained aliases inside the aggregates. On main and 8.x, this is effectively an illegal state: since #104958, aliases in the aggregates become Eval nodes before and after the Aggregate node.

But here, on 8.x, we'll just fix this code path so that it behaves exactly as before #117699.

If this passes the full-bwc test, I plan to forward-port this by removing the obsolete code path on main.

@alex-spies alex-spies added >test-failure Triaged test failures from CI test-full-bwc Trigger full BWC version matrix tests :Analytics/ES|QL AKA ESQL v8.18.0 labels Dec 2, 2024
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) needs:risk Requires assignment of a risk label (low, medium, blocker) labels Dec 2, 2024
@elasticsearchmachine
Copy link
Collaborator

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

This was referenced Dec 2, 2024
Closed
Merged
ivancea
ivancea reviewed Dec 2, 2024
View reviewed changes
...l/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be merged to main and then backported? To avoid conflicts, and ensure we support "everything everywhere".

I see you commented to fix it here, and then remove the obsolete path in main. But I wonder if it could cause issues in the future. Also, if it fails in <8.13, shouldn't 9.x also support it? Or is 8.13 the last compatible 8.x version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see the other PR is identical

...l/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java
for (NamedExpression agg : aggregates) {
if (agg instanceof Alias a) {
if (a.child() instanceof Attribute attr) {
if (groupAttribute.id().equals(attr.id())) {
if (sourceGroupAttribute.id().equals(attr.id())) {
Copy link
Contributor

@ivancea ivancea Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceGroupAttribute == groupAttribute for anything not using Categorize. So, how was this failing for non-Categorize cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line I just updated for alignment with the rest. It's the change below that actually fixes things.

alex-spies
alex-spies commented Dec 2, 2024
View reviewed changes
...l/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java
Comment on lines -136 to 142
if (groupAttribute.semanticEquals(a.toAttribute())) {
groupAttribute = attr;
if (sourceGroupAttribute.semanticEquals(a.toAttribute())) {
sourceGroupAttribute = attr;
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The groupAttribute was mutated, but we use the sourceGroupAttribute to determine the right channel to pass to the groupSpecs. We need to mutate the sourceGroupAttribute instead.

luigidellaquila
luigidellaquila approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good catch @alex-spies

I think this will be fixed as well #117861

@alex-spies alex-spies mentioned this pull request Dec 3, 2024
Closed
@alex-spies alex-spies added >test Issues or PRs that are addressing/adding tests auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed >test-failure Triaged test failures from CI needs:risk Requires assignment of a risk label (low, medium, blocker) labels Dec 3, 2024
ivancea
ivancea approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for quickly finding the issue!

@alex-spies alex-spies removed the test-full-bwc Trigger full BWC version matrix tests label Dec 3, 2024
@alex-spies
Copy link
Contributor Author

The full-bwc task struggles with the 8.15.6 cases due to upstream issues:

* What went wrong:
Could not determine the dependencies of task ':qa:ccs-rolling-upgrade-remote-cluster:v8.15.6#fullUpgraded'.
> Could not resolve all dependencies for configuration ':qa:ccs-rolling-upgrade-remote-cluster:es_distro_extracted_testclusters-qa-ccs-rolling-upgrade-remote-cluster-v8.15.6-remote-0-8.15.6'.
   > Could not find elasticsearch-distribution:elasticsearch:8.15.6.
     Searched in the following locations:
       - https://artifacts-no-kpi.elastic.co/downloads/elasticsearch/elasticsearch-8.15.6-linux-x86_64.tar.gz
     Required by:
         project :qa:ccs-rolling-upgrade-remote-cluster

I think it's better to have this fix in + un-skip the tests, esp. since the affected code path should only matter to 8.11/8.12; therefore, I'll try to re-run the CI without full bwc; it already passed the bwc checks for everything except 8.15.

alex-spies added a commit that referenced this pull request Dec 3, 2024
@alex-spies
ESQL: Fix layout when aggregating with aliases (#117837)
cf9687f 
Forward-port of #117832

Only really relevant for bwc with 8.11/8.12.; port for consistency with 8.x
@elasticsearchmachine elasticsearchmachine merged commit ca48c37 into elastic:8.x Dec 3, 2024
15 checks passed
@alex-spies alex-spies deleted the fix-old-school-aggs branch December 3, 2024 13:13
This was referenced Dec 3, 2024
Closed
Closed
This was referenced Dec 3, 2024
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivancea ivancea ivancea approved these changes

@luigidellaquila luigidellaquila luigidellaquila approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.18.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alex-spies @elasticsearchmachine @luigidellaquila @ivancea