Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ESQL] Support date nanos in binary comparisions #109992

Closed
Tracked by #109352
not-napoleon opened this issue Jun 20, 2024 · 1 comment · Fixed by #111908
Closed
Tracked by #109352

[ESQL] Support date nanos in binary comparisions #109992

not-napoleon opened this issue Jun 20, 2024 · 1 comment · Fixed by #111908
Assignees
@not-napoleon
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@not-napoleon
Copy link
Member

not-napoleon commented Jun 20, 2024 •
edited
Loading

Description

Comparing date nanos to other date nanos values is well defined since they're both longs with the same units. For an initial version, I think we should only support comparing to millisecond dates via an explicit cast (we have another ticket for basic casting support already). There's also a discussion issue for autocasting
@not-napoleon not-napoleon mentioned this issue Jun 20, 2024
Open
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 20, 2024
@not-napoleon not-napoleon mentioned this issue Jun 20, 2024
Closed
@not-napoleon not-napoleon self-assigned this Aug 14, 2024
@not-napoleon not-napoleon mentioned this issue Aug 14, 2024
Merged
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Sep 4, 2024
@not-napoleon @cbuescher
[ESQL] date nanos binary comparisons (elastic#111908)
7411578 
resolves elastic#109992

Nothing fancy here. Nanosecond dates are still longs, and we can just compare them as longs. Please note that, as mentioned in the linked issue, this only supports comparing date nanos to other date nanos, and not comparing to millisecond dates. With the cast functions added in elastic#111850, users can explicitly cast to millisecond dates (or longs) to compare nanos to other things.
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this issue Sep 5, 2024
@not-napoleon @davidkyle
[ESQL] date nanos binary comparisons (elastic#111908)
66a4d12 
resolves elastic#109992

Nothing fancy here. Nanosecond dates are still longs, and we can just compare them as longs. Please note that, as mentioned in the linked issue, this only supports comparing date nanos to other date nanos, and not comparing to millisecond dates. With the cast functions added in elastic#111850, users can explicitly cast to millisecond dates (or longs) to compare nanos to other things.
@not-napoleon not-napoleon mentioned this issue Nov 6, 2024
Merged
not-napoleon added a commit that referenced this issue Nov 7, 2024
@not-napoleon
[ESQL] Fix Binary Comparisons on Date Nanos (#116346)
3b2021f 
Relates to #109992

When I implemented binary comparisons for date nanos in #111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
not-napoleon added a commit to not-napoleon/elasticsearch that referenced this issue Nov 7, 2024
@not-napoleon
[ESQL] Fix Binary Comparisons on Date Nanos (elastic#116346)
5a08dad 
Relates to elastic#109992

When I implemented binary comparisons for date nanos in elastic#111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
elasticsearchmachine pushed a commit that referenced this issue Nov 7, 2024
@not-napoleon
[ESQL] Fix Binary Comparisons on Date Nanos (#116346) (#116411)
8bef761 
Relates to #109992

When I implemented binary comparisons for date nanos in #111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
kderusso pushed a commit to kderusso/elasticsearch that referenced this issue Nov 7, 2024
@not-napoleon @kderusso
[ESQL] Fix Binary Comparisons on Date Nanos (elastic#116346)
896edda 
Relates to elastic#109992

When I implemented binary comparisons for date nanos in elastic#111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
jozala pushed a commit that referenced this issue Nov 13, 2024
@not-napoleon @jozala
[ESQL] Fix Binary Comparisons on Date Nanos (#116346)
980a8fe 
Relates to #109992

When I implemented binary comparisons for date nanos in #111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this issue Nov 28, 2024
@not-napoleon @alexey-ivanov-es
[ESQL] Fix Binary Comparisons on Date Nanos (elastic#116346)
6b755d8 
Relates to elastic#109992

When I implemented binary comparisons for date nanos in elastic#111908 I failed to update the verifyBinaryComparisons rule, which implements another layer of type checking for binary comparisons, external to the type logic in the Expression classes. Since the unit tests for the expressions don't run the full query processing stack, this bug was invisible to them. Only full integration tests, such as the CSV tests I've added here, can catch this kind of error. The initial PR did not include these because we didn't have enough supporting functions (notably TO_DATE_NANOS) to write the tests.

This PR fixes the issue by adding DATE_NANOS to the list of allowed types for binary comparisons in the verifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@not-napoleon not-napoleon

Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[ESQL] date nanos binary comparisons not-napoleon/elasticsearch
2 participants
@not-napoleon @elasticsearchmachine