Skip to content

Parse composite after_key eagerly#94979

Merged
romseygeek merged 2 commits intoelastic:mainfrom
romseygeek:aggs/composite-key-early-parse
Apr 3, 2023
Merged

Parse composite after_key eagerly#94979
romseygeek merged 2 commits intoelastic:mainfrom
romseygeek:aggs/composite-key-early-parse

Conversation

@romseygeek
Copy link
Copy Markdown
Contributor

Composite after_keys are currently only formatted when they are serialized
to xcontent, meaning that a key that cannot be formatted will not be caught
until after all the results from an aggregation have been reduced. This
wastes CPU time, and also blocks any move to chunked xcontent handling
in search responses.

This commit moves formatting of an after_key into InternalComposite's
constructor, catching these errors earlier.

Resolves #94760

@romseygeek romseygeek requested a review from not-napoleon April 3, 2023 09:17
@romseygeek romseygeek self-assigned this Apr 3, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 3, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@romseygeek
Copy link
Copy Markdown
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

Copy link
Copy Markdown
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

I think this makes sense. Thanks for taking care of it.

@romseygeek romseygeek merged commit 092fa8d into elastic:main Apr 3, 2023
@romseygeek romseygeek deleted the aggs/composite-key-early-parse branch April 3, 2023 13:23
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Apr 4, 2023
…94979)"

This reverts commit 092fa8d.

We can't throw exceptions during construction from a StreamInput, as
this can kill nodes in mixed clusters.
romseygeek added a commit that referenced this pull request Apr 4, 2023
…#95015)

This reverts commit 092fa8d.

We can't throw exceptions during construction from a StreamInput, as
this can kill nodes in mixed clusters.
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Format checking in InternalComposite should be done earlier

3 participants