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 timezone_hour/timezone_minute functions #9949

Merged
merged 1 commit into from
Dec 9, 2022
Merged

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Dec 8, 2022

NOTE: this is technically a backwards incompatible change, but the previous behavior was bogus so I hope this is OK (if you were to try to use it the way it's documented it would return completely wrong values)

The original code returned incorrect values (the hours were basically mod 24, instead of returning negative when it should).

Confirmed behavior with Postgres (note that postgres just returns the current timezone because all timestamps are stored in UTC, it uses the UTC timestamp to figure out if it's daylight savings or standard time):

set timezone='America/Toronto';
SELECT EXTRACT(timezone_hour FROM TIMESTAMP WITH TIME ZONE '2022-01-01 6:23:01-08') FROM foo;
 extract
---------
      -5
SELECT EXTRACT(timezone_hour FROM TIMESTAMP WITH TIME ZONE '2022-07-01 6:23:01-08') FROM foo;
 extract
---------
      -4
(1 row)
----------------------------------------
----------------------------------------

set timezone='Etc/GMT+12';
SELECT EXTRACT(timezone_hour FROM TIMESTAMP WITH TIME ZONE '2022-01-01 6:23:01-08') FROM foo;
 extract
---------
     -12
(1 row)
----------------------------------------
----------------------------------------

set timezone='Asia/Shanghai';
SELECT EXTRACT(timezone_hour FROM TIMESTAMP WITH TIME ZONE '2022-07-01 6:23:01-08') FROM foo;
 extract
---------
       8
(1 row)

@Jackie-Jiang Jackie-Jiang added backward-incompat Referenced by PRs that introduce or fix backward compat issues bugfix labels Dec 8, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #9949 (6877bce) into master (e6a9881) will decrease coverage by 53.11%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #9949       +/-   ##
=============================================
- Coverage     68.97%   15.85%   -53.12%     
+ Complexity     5058      176     -4882     
=============================================
  Files          1982     1931       -51     
  Lines        106433   104308     -2125     
  Branches      16127    15901      -226     
=============================================
- Hits          73409    16542    -56867     
- Misses        27877    86545    +58668     
+ Partials       5147     1221     -3926     
Flag Coverage Δ
integration2 ?
unittests1 ?
unittests2 15.85% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...inot/common/function/scalar/DateTimeFunctions.java 0.00% <0.00%> (-95.72%) ⬇️
...src/main/java/org/apache/pinot/sql/FilterKind.java 0.00% <0.00%> (-100.00%) ⬇️
...ain/java/org/apache/pinot/core/data/table/Key.java 0.00% <0.00%> (-100.00%) ⬇️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...n/java/org/apache/pinot/core/data/table/Table.java 0.00% <0.00%> (-100.00%) ⬇️
.../java/org/apache/pinot/core/data/table/Record.java 0.00% <0.00%> (-100.00%) ⬇️
.../java/org/apache/pinot/core/util/GroupByUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1470 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

lol i run into the exact same issue where adding tests.

@walterddr walterddr merged commit 60a62fe into apache:master Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompat Referenced by PRs that introduce or fix backward compat issues bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants