SQL: Fix the inconsistent behaviour of ISO_WEEK_YEAR() #68758
Merged
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
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of elastic#67872
palesz
added
>bug
:Analytics/SQL
|
Pinging @elastic/es-ql (Team:QL) |
matriv
approved these changes
Feb 9, 2021
| @@ -131,6 +131,32 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp ORDER BY WEEK(birth_date) | |||
| 44 |1961-11-02T00:00:00.000Z | |||
| ; | |||
|
|
|||
| isoWeekOfYear | |||
| schema::birth_date:ts|iso_week:i|week:i | |||
| SELECT birth_date, IW(birth_date) iso_week, WEEK(birth_date) week FROM test_emp WHERE IW(birth_date) < 8 ORDER BY iso_week; | |||
There was a problem hiding this comment.
Minor: maybe you can add a AND week > 2 so we can check both filters work and one filter uses also the alias`
bpintea
approved these changes
Feb 9, 2021
| ---------------+--------------- | ||
| 1 |2 |
There was a problem hiding this comment.
Note to self: "manual" checking the csv specs sometimes pays off.
costin
approved these changes
Feb 10, 2021
| @@ -221,14 +221,14 @@ public static Number tan(Number value) { | |||
| // | |||
| // Date/Time functions | |||
| // | |||
| public static Integer dateTimeChrono(Object dateTime, String tzId, String chronoName) { | |||
| if (dateTime == null || tzId == null || chronoName == null) { | |||
| public static Integer dateTimeExtract(Object dateTime, String tzId, String extractorName) { | |||
There was a problem hiding this comment.
I don't have a strong preference regarding the method rename however there is an associated cost for it in backwards compatibility.
When executing across older clusters, the generated query would fail since there's no dateTimeExtract in those versions, only dateTimeChrono.
To avoid this, simply keep dateTimeChrono around and redirect it to dateTimeExtract so its backwards compatible or avoid the method rename in the first place.
There was a problem hiding this comment.
Thanks, good catch. It is not just a simple method rename, also the possible values of the last parameter changed (from ChronoField enum to DateTimeExtract enum values). I will keep around a @Deprecated dateTimeChrono() for backwards compatibility that will do this translation. I guess we can remove it in the next major version.
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 11, 2021
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of elastic#67872
palesz
pushed a commit
to palesz/elasticsearch
that referenced
this pull request
Feb 11, 2021
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of elastic#67872
palesz
pushed a commit
that referenced
this pull request
Feb 11, 2021
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of #67872
palesz
pushed a commit
that referenced
this pull request
Feb 11, 2021
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of #67872
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
SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4query returnedwith
x=3results because theISO_WEEK_YEAR(a)in the WHERE clausethat turns into a script query and the
ISO_WEEK_YEAR(a)in the projectionsthat turns into a post-processing on top of the Query DSL results execute
different code to calculate the result.
This change unifies the different code paths and results in a single method
being responsible for the actual calculation.
Note: this change impacts the way how all the
DateTimeFunctions thatdo the field extraction from a date get translated into a script query.
Fixes part of #67872