-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[ESQL] Consolidate EsqlProject into Project #140238
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
Conversation
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Merges the EsqlProject class into Project, eliminating unnecessary class hierarchy after the QL merge. Key changes: - Move lenient expressionsResolved() behavior from EsqlProject to Project, allowing UnsupportedAttribute to pass through projections unchanged - Add ESQL_PROJECT_ENTRY to PlanWritables for backward compatibility when deserializing old "EsqlProject" plans from mixed-version clusters - Add checkUnsupportedAttributeRenaming() to Verifier to ensure renaming UnsupportedAttribute via Alias is still blocked (this check now runs unconditionally, not gated by resolved() status) - Update all instantiation sites to use Project instead of EsqlProject - Update comments referencing EsqlProject Closes elastic#111957
- Replace all EsqlProject references with Project in test assertions - Remove EsqlProjectSerializationTests (no longer needed) - Add regression tests in VerifierTests to validate that: - UnsupportedAttribute can pass through KEEP (Project) unchanged - Renaming UnsupportedAttribute fails even after passing through KEEP
61512a8 to
199dd85
Compare
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/local/EsqlProject.java
Show resolved
Hide resolved
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/local/EsqlProject.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Outdated
Show resolved
Hide resolved
idegtiarenko
left a comment
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.
Left couple of suggestions, overall looks good to me.
I am not changing this area often so it would be nice to have one extra review for this change.
ivancea
left a comment
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.
Thanks, LGTM!
…plan/logical/Project.java Co-authored-by: Ievgen Degtiarenko <[email protected]>
…plan/logical/Project.java Co-authored-by: Ievgen Degtiarenko <[email protected]>
4eabb62 to
d819361
Compare
We warn for unhandled annotations because of codegen
Test assertions will fail during deser if we create a `Project` with the name `EsqlProject` (old -> new transmission)
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Show resolved
Hide resolved
.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/MarkerAnnotationProcessor.java
Outdated
Show resolved
Hide resolved
.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/MarkerAnnotationProcessor.java
Show resolved
Hide resolved
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.
Should this be moved with the annotation in :libs:core?
Depending on if we can use UpdateForV10.class.getCanonicalName(), this processor may be difficult to find. Unless it fails on compile time anyway, which could work I guess.
astefan
left a comment
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.
LGTM
| /** | ||
| * Backward compatibility entry + name for reading the consolidated `EsqlProject` plans from pre-9.4.0 nodes. | ||
| */ | ||
| private static final String LEGACY_PROJECT_NAME = "EsqlProject"; |
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.
Nit: If this is something that is targeted to ESQL team, I think LEGACY_LOGICAL_PLAN_NAME describes better the constant and intention.
alex-spies
left a comment
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.
LGTM. Thanks @MattAlp !
|
Ah, @MattAlp , because this affects serialization, you may want to run the CI with the |
This consolidates the
EsqlProjectclass intoProject, eliminating unnecessary class hierarchy originating from the QL merge. We've been meaning to do this for a while, #139248 did a lot of similar heavy lifting recently.The changes themselves:
expressionsResolved()behavior fromEsqlProjectintoProject, allowingUnsupportedAttributeto pass through projections unchangedESQL_PROJECT_ENTRYtoPlanWritablesfor backward compatibility when deserializing old"EsqlProject"plans from mixed-version clusterscheckUnsupportedAttributeRenaming()toVerifierto ensure renamingUnsupportedAttributeviaAliasis still blocked (this check now runs unconditionally, not gated byresolved()status)UnsupportedAttributepass-through and rename-blocking behaviorCloses #109195