-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] Take date date_nanos implicit casting out of snapshot #133369
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
[ES|QL] Take date date_nanos implicit casting out of snapshot #133369
Conversation
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.
Pull Request Overview
This PR removes the snapshot requirement for the implicit casting functionality between date and date_nanos types in ES|QL, making it available in production builds. The feature was previously gated behind a snapshot-only capability flag.
- Removes snapshot gating from the
IMPLICIT_CASTING_DATE_AND_DATE_NANOS
capability - Removes conditional test assumptions that required snapshot builds
- Simplifies the
DateMillisToNanosInEsRelation
analyzer rule by removing snapshot checks
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
160_union_types.yml | Adds aggregate_metric_double capability to test requirements |
QueryTranslatorTests.java | Removes snapshot assumption from date_nanos test |
LocalPhysicalPlanOptimizerTests.java | Removes snapshot assumption from date_nanos pushdown test |
AnalyzerTests.java | Removes snapshot assumption from implicit casting test |
Analyzer.java | Simplifies DateMillisToNanosInEsRelation rule by removing snapshot conditionals |
EsqlCapabilities.java | Removes snapshot requirement from IMPLICIT_CASTING_DATE_AND_DATE_NANOS capability |
RestEsqlIT.java | Removes snapshot check from suggested cast test logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Hi @fang-xing-esql, I've created a changelog YAML for you. |
🔍 Preview links for changed docs |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
The failed release tests are not related to this PR. @alex-spies Could you help review this? This feature is ready to be taken out of snapshot according to the last few comments here Thank 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.
Hiya, this looks good. Some additional suggestions for the docs, but as long as the (corresponding) release tests pass this should be good to go :)
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
Show resolved
Hide resolved
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/160_union_types.yml
Show resolved
Hide resolved
Also, sorry, qualifiers caused a conflict 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.
Very nice docs! Just couple very minor suggestions from me :)
ℹ️ 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?
|
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.
We need to version tag the docs nowadays :)
release tests passed locally. The |
Thanks so much for reviewing @alex-spies @leemthompo ! |
This PR takes #127797 out of snapshot. It was gated behind snapshot by #130026 .
This was suggested originally by this issue - #110009.
Just want to provide some background information here again.
The goal of #127797 is to support union typed fields that reference both
date
anddate_nanos
fields without explicit casting. Union typed(mixed withdate
anddate_nanos
) fields are casted todate_nanos
during Analyzer phase automatically. numeric or string typed fields are not within the scope of this feature.Without this feature(release mode), queries that reference union(mixed with
date
anddate_nanos
) typed fields like below either return nulls or fail like the examples below, because they reference union typed fields without explicit casting.With this feature(already behind snapshot), they should not fail or return null for values within
date_nanos
's valid ranges.