Add checks that optimizers do not modify the layout #130855
Conversation
julian-elastic
force-pushed
the
outputChecker_v2
branch
3 times, most recently
from
5702aa0 to
7ec9efc
Compare
julian-elastic
force-pushed
the
outputChecker_v2
branch
from
7ec9efc to
4d69b79
Compare
elasticsearchmachine
added
the
needs:triage
elasticsearchmachine
added
the
Team:Analytics
|
Hi @julian-elastic, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
removed
the
needs:triage
|
Hi @julian-elastic, I've updated the changelog YAML for you. |
There was a problem hiding this comment.
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. |
...k/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java
Outdated
Show resolved
Hide resolved
|
|
||
| import static org.elasticsearch.xpack.esql.common.Failure.fail; | ||
|
|
||
| /** Physical plan verifier. */ | ||
| public final class PhysicalVerifier { | ||
| public final class PhysicalVerifier extends PostOptimizationPhasePlanVerifier { |
There was a problem hiding this comment.
nit: Can the superclass receive a generic with the type of plan?
It's not very important now, but lines like optimizedPlan.collectFirstChildren(EnrichExec.class::isInstance); should fail if you write Enrich.class by mistake (For example). But as it's a QueryPlan, it won't now
.../src/main/java/org/elasticsearch/xpack/esql/optimizer/PostOptimizationPhasePlanVerifier.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/esql/optimizer/PostOptimizationPhasePlanVerifier.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java
Show resolved
Hide resolved
...sql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java
Outdated
Show resolved
Hide resolved
...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks super good! However, can we merge #129745 first, so the fix to ProjectAwayColumns is in a separate PR (which we'll backport more aggressively)?
Other than that this LGTM!
.../src/main/java/org/elasticsearch/xpack/esql/optimizer/PostOptimizationPhasePlanVerifier.java
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java
Outdated
Show resolved
Hide resolved
!
Add verification that the optimizers do not modify the number of attributes and the attribute datatype.
We add special handling for Lookup Join, by checking EsQueryExec esQueryExec && esQueryExec.indexMode() == LOOKUP and another special handling for ProjectAwayColumns.ALL_FIELDS_PROJECTED
Closes #125576