SQL: Fix the MINUTE_OF_DAY() function that throws exception when used in comparisons #68783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… in comparisons The `MINUTE_OF_DAY()` extraction function does not have an equivalent expressable using a datetime format pattern. The `MinuteOfDay.dateTimeFormat()` is called during the query translation and throws an exception, but the return value actually does not impact the translated query (binary comparisons with `DateTimeFunction` on one side always turn into a script query). This change fixes the immediate issue raised as part of elastic#67872, add integration tests covering the problem, but leaves the removal of the unnecessary `dateTimeFormat()` function a separate PR.
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 9, 2021
…ranslations lazy Removes the `DateTimeFunction.dateTimeFormat()` method that was called, but had no effect on the translated queries. This change also adds laziness, so translations don't execute unnecessarily. Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows elastic#68783
palesz
added
:Analytics/SQL
|
Pinging @elastic/es-ql (Team:QL) |
|
https://github.com/elastic/elasticsearch/pull/68788/files#diff-385222df7bc48aa29c7282fd51bd3f3de071de85974214e19a44670750637182R274 and https://github.com/elastic/elasticsearch/pull/68788/files#diff-4f9289c27c5213594234e5b9a58c9a35248e71bb33b80696c6c6fddd13ea592dR45 shows the places where the ScriptQuery "wins" and throws away the results of the queries that were the only consumers of the |
bpintea
approved these changes
Feb 10, 2021
| @@ -153,6 +153,16 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp WHERE WEEK(birth_date) > | |||
| 2 |1953-01-07T00:00:00.000Z | |||
| ; | |||
|
|
|||
| minuteOfDayFilterEquality | |||
| SELECT MINUTE_OF_DAY(CONCAT(CONCAT('2021-01-22T14:26:06.', (salary % 2)::text), 'Z')::datetime) AS min_of_day | |||
There was a problem hiding this comment.
Not sure if adding the given time to the dates in hire_date would have been less contrived, but this works too.
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 10, 2021
… in comparisons (elastic#68783) The `MINUTE_OF_DAY()` extraction function does not have an equivalent expressable using a datetime format pattern. The `MinuteOfDay.dateTimeFormat()` is called during the query translation and throws an exception, but the return value actually does not impact the translated query (binary comparisons with `DateTimeFunction` on one side always turn into a script query). This change fixes the immediate issue raised as part of elastic#67872, add integration tests covering the problem, but leaves the removal of the unnecessary `dateTimeFormat()` function a separate PR.
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 10, 2021
… in comparisons (elastic#68783) The `MINUTE_OF_DAY()` extraction function does not have an equivalent expressible using a datetime format pattern. The `MinuteOfDay.dateTimeFormat()` is called during the query translation and throws an exception, but the return value actually does not impact the translated query (binary comparisons with `DateTimeFunction` on one side always turn into a script query). This change fixes the immediate issue raised as part of elastic#67872, add integration tests covering the problem, but leaves the removal of the unnecessary `dateTimeFormat()` function a separate PR.
palesz
pushed a commit
that referenced
this pull request
Feb 10, 2021
… in comparisons (#68783) (#68846) The `MINUTE_OF_DAY()` extraction function does not have an equivalent expressible using a datetime format pattern. The `MinuteOfDay.dateTimeFormat()` is called during the query translation and throws an exception, but the return value actually does not impact the translated query (binary comparisons with `DateTimeFunction` on one side always turn into a script query). This change fixes the immediate issue raised as part of #67872, add integration tests covering the problem, but leaves the removal of the unnecessary `dateTimeFormat()` function a separate PR.
palesz
pushed a commit
that referenced
this pull request
Feb 10, 2021
… in comparisons (#68783) (#68854) The `MINUTE_OF_DAY()` extraction function does not have an equivalent expressible using a datetime format pattern. The `MinuteOfDay.dateTimeFormat()` is called during the query translation and throws an exception, but the return value actually does not impact the translated query (binary comparisons with `DateTimeFunction` on one side always turn into a script query). This change fixes the immediate issue raised as part of #67872, add integration tests covering the problem, but leaves the removal of the unnecessary `dateTimeFormat()` function a separate PR.
palesz
pushed a commit
that referenced
this pull request
Feb 17, 2021
Removes the `DateTimeFunction.dateTimeFormat()` and the `TranslatorHandler.dateTimeFormat()` methods that were called, but had no effect on the translated queries. One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows #68783
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 17, 2021
…ic#68788) Removes the `DateTimeFunction.dateTimeFormat()` and the `TranslatorHandler.dateTimeFormat()` methods that were called, but had no effect on the translated queries. One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows elastic#68783
palesz
pushed a commit
that referenced
this pull request
Feb 18, 2021
… (#69123) Removes the `DateTimeFunction.dateTimeFormat()` and the `TranslatorHandler.dateTimeFormat()` methods that were called, but had no effect on the translated queries. One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows #68783
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 18, 2021
Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows elastic#68783
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Mar 2, 2021
Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows elastic#68783 and elastic#68788
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Mar 2, 2021
The `DateTimeFunction.dateTimeFormat()` method that was called, but had no effect on the translated queries. One of the examples, when this happens, is when one side of the `BinaryComparison` is a function. The `BinaryComparison` at the end turns into a `ScriptQuery`, but in the intermediate steps the `ExpressionTranslators` try to translate it to a `RangeQuery`. Follows elastic#68783
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
MINUTE_OF_DAY()extraction function does not have an equivalentdatetime format pattern.
The
MinuteOfDay.dateTimeFormat()is called during the querytranslation and throws an exception, but the return value actually
does not impact the translated query (binary comparisons with
DateTimeFunctionon one side always turn into a script query).This change fixes the immediate issue raised as part of #67872,
add integration tests covering the problem, but leaves the removal
of the unnecessary
dateTimeFormat()function to #68788.