[ES|QL] Pushdown string format for BinaryComparison on datetime fields#110669
[ES|QL] Pushdown string format for BinaryComparison on datetime fields#110669fang-xing-esql merged 7 commits intoelastic:mainfrom
Conversation
|
Hi @fang-xing-esql, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
alex-spies
left a comment
There was a problem hiding this comment.
LGTM, since this fixes some csv tests when run in ITs.
No idea if we should use datetime strings or epoch millis in general, though.
Note: I think it's super unfortunate that we need to change both ExpressionTranslators in core and EsqlExpressionTranslators. But we plan to consolidate these (and their tests) in the future.
| ImplicitCastingEqual | ||
| required_capability: rangequery_for_datetime | ||
| from employees | ||
| | where birth_date == "1957-05-23T00:00:00Z" |
There was a problem hiding this comment.
For good measure, can we add a test with a different time zone than Z, too?
There was a problem hiding this comment.
We don't currently have any support for non-UTC time zones, so such a test would not be expected to pass.
There was a problem hiding this comment.
Yes, we don't support non-UTC time zone yet.
...esql-core/src/main/java/org/elasticsearch/xpack/esql/core/planner/ExpressionTranslators.java
Outdated
Show resolved
Hide resolved
...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsqlExpressionTranslators.java
Outdated
Show resolved
Hide resolved
nik9000
left a comment
There was a problem hiding this comment.
I think this is the right thing to do, but it's kind of a symptom of a problem, one we might should try to fix rather than pushing this mechanism a ton further. If we're on the local node we have the local node we can get the MappedFieldType and talk to it directly. Or we can just make Lucene Query subclasses. Right now we can't do that and we have to go through a few layers - one of which prefers iso8601 for date comparisons. Which is how we got here. I think. But if we didn't loop through all of these layers it'd be more obvious what's going on.
There was a problem hiding this comment.
The RangeQuery accepts a format parameter, that, if specified as "epoch_millis", will match as expected also when provided the millis that we currently provide it.
However, the term query does not. So providing there a string is the right thing (would be great to have it take a format, but not sure why this isn't the case, maybe the matching is done differently).
But I think we shouldn't turn an equality into a RangeQuery, we should rather just provide term with the string representation of the date.
Also, if we can keep the number format and provide a "format" to the inequalities, that'd be great; if not, string also here.
|
#109413 moved InComparison from esql-core into |
Thanks for reviewing @bpintea ! We'll change datetime literals to string only, and keep the term query. |
bpintea
left a comment
There was a problem hiding this comment.
Thanks, Fang, good find!
#110669) Use string format in datetime field BinaryComparison.
#110669) Use string format in datetime field BinaryComparison.
Resolves #107900
Equal and IN list predicates on datetime fields may return wrong results. This PR fixes the wrong results by using String representation of the datetime literals/constants in BinaryComparison on datetime fields.