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: date_bin() on timstamps before 1970 #13204

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Conversation

mhilton
Copy link
Contributor

@mhilton mhilton commented Oct 31, 2024

Which issue does this PR close?

Closes #13145

Rationale for this change

The date_bin() function was not working correctly for timestamps before 1970. Specifically if the input timestamp was the exact time of the start of a bin then it would be placed in the previous bin.

What changes are included in this PR?

The % operator has a negative result when the dividend is negative. This causes the date_bin calculation to round up to the next bin. To compensate the size of 1 interval is subtracted from the result if the input is negative. This subtraction is no longer performed if the input is already the exact time of the start of a bin.

Are these changes tested?

Yes I've added unit tests and an SQL logic test.

Are there any user-facing changes?

None, other than the bug fix itself.

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Oct 31, 2024
The date_bin() function was not working correctly for timestamps before
1970. Specifically if the input timestamp was the exact time of the
start of a bin then it would be placed in the previous bin.

The % operator has a negative result when the dividend is negative.
This causes the date_bin calculation to round up to the next bin. To
compensate the size of 1 interval is subtracted from the result if the
input is negative. This subtraction is no longer performed if the input
is already the exact time of the start of a bin.
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Makes sense to me -- thank you @mhilton

@alamb
Copy link
Contributor

alamb commented Nov 1, 2024

Thanks again @mhilton

@alamb alamb merged commit a2e5330 into apache:main Nov 1, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date_bin: incorrect binning at date_bin boundaries before the epoch
2 participants