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

Configure default timezone #449

Merged
merged 26 commits into from
Sep 29, 2021
Merged

Configure default timezone #449

merged 26 commits into from
Sep 29, 2021

Conversation

lziq
Copy link
Contributor

@lziq lziq commented Sep 28, 2021

Issue #410

Solution Description:

We want to provide an option to configure default timezone offset through EvaluationSession class, since the default timezone offset should be considered as a kind of information provided when we evaluate the time.

Changes Details:

Source code:

  1. Added the new attribute defaultTimezoneOffset to EvaluationSession class. The default value is set to be UTC time.
  2. Removed the global field DEFAULT_TIMEZONE_OFFSET.
  3. Modified the source code for time evaluation and time cast, so that the default timezone is now provided by EvaluationSession.

Test:

  1. Added test for defaultTimezoneOffset for EvaluationSession class.
  2. Added test for time evaluation.
  3. Added test for time cast.
  4. Created a local variable defaultTimezoneOffset to replace DEFAULT_TIMEZONE_OFFSET for test cases using it.

@lziq lziq linked an issue Sep 28, 2021 that may be closed by this pull request
@lziq lziq self-assigned this Sep 28, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2021

Codecov Report

Merging #449 (b7ee8bf) into main (6301396) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #449   +/-   ##
=========================================
  Coverage     82.35%   82.36%           
- Complexity     1394     1395    +1     
=========================================
  Files           171      171           
  Lines         10717    10722    +5     
  Branches       1766     1766           
=========================================
+ Hits           8826     8831    +5     
  Misses         1350     1350           
  Partials        541      541           
Flag Coverage Δ
CLI 18.18% <ø> (ø)
EXAMPLES 74.85% <ø> (ø)
LANG 84.86% <88.88%> (+<0.01%) ⬆️
PTS ∅ <ø> (∅)
TEST_SCRIPT 79.68% <ø> (ø)

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

Impacted Files Coverage Δ
...ng/src/org/partiql/lang/eval/EvaluatingCompiler.kt 84.22% <ø> (ø)
...g/src/org/partiql/lang/eval/ExprValueExtensions.kt 84.71% <50.00%> (ø)
...ang/src/org/partiql/lang/eval/EvaluationSession.kt 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6301396...b7ee8bf. Read the comment docs.

Copy link
Member

@dlurton dlurton left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, just one minor change requested. Thanks.

lang/src/org/partiql/lang/eval/ExprValueExtensions.kt Outdated Show resolved Hide resolved
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Only had a few minor comments. Looks good overall!

Copy link
Contributor

@abhikuhikar abhikuhikar left a comment

Choose a reason for hiding this comment

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

EvaluationSession class also has a now: Timestamp member. It can have its own time zone offset defined. I was wondering if it would conflict with a separate default time zone. Should we consider using now.localOffset which is time zone offset in minutes? (It can be null though). Need to investigate more into this.

For the PR, just one minor suggestion. Otherwise looks great to me.

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

Nice job :shipit:

@lziq lziq merged commit 28701e2 into main Sep 29, 2021
@lziq lziq deleted the configure-default-timezone branch September 29, 2021 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an option to configure default time zone
5 participants