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.

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

Fix issues with WEEK/ISO_WEEK/DATEDIFF #49405

Merged
merged 3 commits into from
Nov 29, 2019
Merged

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Nov 20, 2019

Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

  • Non-iso WEEK seems to be broken since [Java.time] Calculate week of a year with ISO rules #48209 because
    of the replacement of Calendar and the change in the ISO rules.

  • ISO_WEEK failed for some edge cases around the January 1st.

  • DATE_DIFF was previously wrongly calculated based on
    non-iso WEEK extraction.

Fixes: #49376

@matriv matriv added >bug :Analytics/SQL SQL querying labels Nov 20, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

Non-iso WEEK seems to be broken since elastic#48209 because
of the replacement of Calendar and the change in the ISO rules.

ISO_WEEK failed for some edge cases around the January 1st.

DATE_DIFF was previously based on non-iso WEEK extraction which seems
not to be the case.

Fixes: elastic#49376
@matriv
Copy link
Contributor Author

matriv commented Nov 27, 2019

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

@matriv matriv requested a review from bpintea November 27, 2019 19:17
@matriv
Copy link
Contributor Author

matriv commented Nov 27, 2019

@elasticmachine run elasticsearch-ci/bwc

@matriv
Copy link
Contributor Author

matriv commented Nov 27, 2019

@elasticmachine run elasticsearch-ci/default-distro

@matriv matriv changed the title SQL: Fix issues with WEEK/ISO_WEEK SQL: Fix issues with WEEK/ISO_WEEK/DATEDIFF Nov 27, 2019
@matriv
Copy link
Contributor Author

matriv commented Nov 28, 2019

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

@matriv
Copy link
Contributor Author

matriv commented Nov 28, 2019

@elasticmachine run elasticsearch-ci/bwc

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

diffWeeks += extraWeek;
}
return safeInt(diffWeeks);
long startInDays = start.toInstant().toEpochMilli() / DAY_IN_MILLIS -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short and clear.

@matriv matriv merged commit 54fe7f5 into elastic:master Nov 29, 2019
@matriv matriv deleted the fix-49376 branch November 29, 2019 16:06
matriv added a commit that referenced this pull request Nov 29, 2019
Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

Non-iso WEEK seems to be broken since #48209 because
of the replacement of Calendar and the change in the ISO rules.

ISO_WEEK failed for some edge cases around the January 1st.

DATE_DIFF was previously based on non-iso WEEK extraction which seems
not to be the case.

Fixes: #49376

(cherry picked from commit 54fe7f5)
matriv added a commit that referenced this pull request Nov 29, 2019
Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

Non-iso WEEK seems to be broken since #48209 because
of the replacement of Calendar and the change in the ISO rules.

ISO_WEEK failed for some edge cases around the January 1st.

DATE_DIFF was previously based on non-iso WEEK extraction which seems
not to be the case.

Fixes: #49376

(cherry picked from commit 54fe7f5)
matriv added a commit that referenced this pull request Nov 29, 2019
Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

Non-iso WEEK seems to be broken since #48209 because
of the replacement of Calendar and the change in the ISO rules.

ISO_WEEK failed for some edge cases around the January 1st.

DATE_DIFF was previously based on non-iso WEEK extraction which seems
not to be the case.

Fixes: #49376

(cherry picked from commit 54fe7f5)
@jpountz jpountz changed the title SQL: Fix issues with WEEK/ISO_WEEK/DATEDIFF Fix issues with WEEK/ISO_WEEK/DATEDIFF Dec 18, 2019
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
Some extended testing with MS-SQL server and H2 (which agree on
results) revealed bugs in the implementation of WEEK related extraction
and diff functions.

Non-iso WEEK seems to be broken since elastic#48209 because
of the replacement of Calendar and the change in the ISO rules.

ISO_WEEK failed for some edge cases around the January 1st.

DATE_DIFF was previously based on non-iso WEEK extraction which seems
not to be the case.

Fixes: elastic#49376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: incorrect ISO_WEEK_OF_YEAR and WEEK_OF_YEAR
5 participants