-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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] Convert string to datetime when the other size of an arithmetic operator is date_period or time_duration #108455
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @fang-xing-esql, I've created a 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.
Thanks @fang-xing-esql , looks good!
I'd increase test coverage a bit, though; maybe we could also add an analyzer/verifier test that confirms that arbitrary functions producing a (foldable) string can't be used, so that e.g. mv_concat("2024", "-04", "-01") + 1 day
is invalid.
x-pack/plugin/esql/qa/testFixtures/src/main/resources/date.csv-spec
Outdated
Show resolved
Hide resolved
Pinging @elastic/kibana-esql (ES|QL-ui) |
Thanks for reviewing @alex-spies ! More tests are added as suggested. |
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 @fang-xing-esql, looking good!
Tiny suggestion for one more test, but this is already good to go as-is.
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.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.
LGTM
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.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.
Lgtm.
I'd add two more tests (on top of all suggested ones): temporal ± string ± temporal and the other way around, string ± temporal ± string
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
Outdated
Show resolved
Hide resolved
Thanks for reviewing @costin @bpintea @alex-spies ! Just need to clean up CI. |
Resolves : #108432
eval x = 1 day + "2024-01-01"
this will work after this change. Users don't need to call to_dt explicitly like thiseval x = 1 day + to_dt("2024-01-01")