Skip to content

Add checks that optimizers do not modify the layout #130855

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

julian-elastic
Copy link
Contributor

@julian-elastic julian-elastic commented Jul 8, 2025

Add verification that the optimizers do not modify the number of attributes and the attribute datatype.
We add an exception for Lookup Join, by checking EsQueryExec esQueryExec && esQueryExec.indexMode() == LOOKUP

Closes #125576

@julian-elastic julian-elastic force-pushed the outputChecker_v2 branch 3 times, most recently from 5702aa0 to 7ec9efc Compare July 9, 2025 17:40
@julian-elastic julian-elastic marked this pull request as ready for review July 10, 2025 20:10
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 10, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 10, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @julian-elastic, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jul 10, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @julian-elastic, I've updated the changelog YAML for you.

Copy link
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.

Looks good! Mostly minor remarks.

The only non-minor remark is on tests: Can we add tests that shows that the verification triggers when optimization changes the plan's output?

One way to test this is using an inherited optimizer instance where we add a final batch which adds a wrong projection on top, which adds/removes some attributes + changes the data types. We could place such a test into the logical and physical optimizer tests, each.

@julian-elastic
Copy link
Contributor Author

@alex-spies This is ready for final review, I addressed the code review feedback. I added the UTs to the LocalLogical and LocalPhysical plan optimizer UTs. I am not sure there is any benefit adding them to the Local and Physical plan optimizer UTs as the code is shared.

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) v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESQL: Enable plan consistency checker for local logical plans
3 participants