-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
base: main
Are you sure you want to change the base?
Conversation
5702aa0
to
7ec9efc
Compare
7ec9efc
to
4d69b79
Compare
Hi @julian-elastic, I've created a changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @julian-elastic, I've updated the changelog YAML for you. |
There was a problem hiding this 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.
...k/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java
Outdated
Show resolved
Hide resolved
@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. |
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