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.

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

ESQL: Add aggregates node level reduction #107876

Merged
merged 11 commits into from
May 9, 2024

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Apr 24, 2024

This adds the remaining aggregate node-level reduction, following #106516.
Also, it estimates the row size for TopN and Aggregate node-level reduction plans.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.15.0 labels Apr 24, 2024
@astefan
Copy link
Contributor Author

astefan commented Apr 25, 2024

@elasticmachine update branch

@astefan astefan added >enhancement :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Apr 25, 2024
@astefan astefan requested review from costin and dnhatn April 25, 2024 08:02
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 25, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@astefan
Copy link
Contributor Author

astefan commented Apr 25, 2024

@elasticmachine update branch

@astefan
Copy link
Contributor Author

astefan commented Apr 25, 2024

@elasticmachine run elasticsearch-ci/bwc-snapshots

@astefan
Copy link
Contributor Author

astefan commented Apr 25, 2024

@elasticmachine run elasticsearch-ci/8.15.0 / bwc-snapshots

@astefan
Copy link
Contributor Author

astefan commented Apr 25, 2024

@elasticmachine update branch

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @astefan.

aggregatorMode = AggregatorMode.INITIAL;
}
} else {
aggregatorMode = AggregatorMode.INITIAL;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be AggregatorMode.SINGLE instead? Can we assert false: ... here since we shouldn't reach this point in our current code base.

if (mode == AggregateExec.Mode.FINAL) {
aggregatorMode = AggregatorMode.FINAL;
} else if (mode == AggregateExec.Mode.PARTIAL) {
if (aggregateExec.child() instanceof ExchangeSourceExec) {// the reducer step at data node (local) level
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there should be a better way to plug this in. This feels like it'd break in a sneaky way one day.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can add a new aggregation mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 created #108468.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@astefan astefan merged commit 1b7cad1 into elastic:main May 9, 2024
15 checks passed
@astefan astefan deleted the add_node_reduction branch May 9, 2024 14:57
markjhoy pushed a commit to markjhoy/elasticsearch that referenced this pull request May 9, 2024
* Add aggregation intermediate reduction level and estimatedRowSize
computed value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants