Skip to content

Simplify PreAnalysisResult#136686

Closed
idegtiarenko wants to merge 2 commits intoelastic:mainfrom
idegtiarenko:simplify_pre_analysis
Closed

Simplify PreAnalysisResult#136686
idegtiarenko wants to merge 2 commits intoelastic:mainfrom
idegtiarenko:simplify_pre_analysis

Conversation

@idegtiarenko
Copy link
Copy Markdown
Contributor

This replaces semi-immutable record with inner static class. This way we could avoid copying it every time we add resolved data saving some object allocation and code verbosity.

@idegtiarenko idegtiarenko added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.3.0 labels Oct 16, 2025
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Do we plan to use the new mutability of PreAnalysisResult in resolveIndicesAndAnalyze? That's where PreAnalysisResult is being the most tedious, as we pass it through the listener chain rather than capturing it in the lambdas and mutating it.

Without that, it doesn't seem like we actuallly simplify the code.

When we merge this, maybe let's also backport to 9.2 - Nik still has some code in flight that will be backported and probably touches the EsqlSession.

@idegtiarenko
Copy link
Copy Markdown
Contributor Author

Outdated

@idegtiarenko idegtiarenko deleted the simplify_pre_analysis branch December 17, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants