Configure default timezone #449

lziq · 2021-09-28T19:22:45Z

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:

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

Test:

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

…rsing phase to evaluating phase.

codecov-commenter · 2021-09-28T19:37:12Z

Codecov Report

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

@@            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.

dlurton

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

lang/src/org/partiql/lang/eval/ExprValueExtensions.kt

…rValue.cast` method.

alancai98

Only had a few minor comments. Looks good overall!

lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

lang/src/org/partiql/lang/eval/EvaluationSession.kt

lang/src/org/partiql/lang/eval/ExprValueExtensions.kt

lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt

lang/test/org/partiql/lang/eval/EvaluatingCompilerDateTimeTests.kt

lang/test/org/partiql/lang/eval/EvaluationSessionTest.kt

Co-authored-by: Alan Cai <caialan@amazon.com>

…s.kt Co-authored-by: Alan Cai <caialan@amazon.com>

…`session` in `ExprValue.cast`.

…est`

…ql/partiql-lang-kotlin into configure-default-timezone

abhikuhikar

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.

lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt

lang/test/org/partiql/lang/eval/EvaluatingCompilerDateTimeTests.kt

lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt

…est cases.

alancai98

Nice job

lziq added 17 commits September 22, 2021 16:19

Changed adding default timezone offset to TIME WITH TIME ZONE from pa…

38f01c6

…rsing phase to evaluating phase.

Remove unnecessary files

ea06c69

Modified comments

9728308

Fix issue 410

49d23b1

Fix issue 410

b86a559

Provide an option in EvaluationSession to configure default timezone.

82147af

Added curly braces for line 2419 in SqlParser

dab08fc

Added "DataTimeFormatter" qualifier for line 2420 and 2423 in SqlParser

2bd6c56

Removed git merge conflict in ask.kt

a407439

Removed git merge conflict in ask.kt

1fe1b27

Changed code style

27e167d

Changed code style

5297a5e

Changed code style

d5bfa8b

Changed code style

6580491

Changed code style

0e5d497

Changed code style

fb754b3

Added more test cases for time evaluation

572acc8

lziq requested review from jpschorr, dlurton, abhikuhikar and alancai98 September 28, 2021 19:22

lziq linked an issue Sep 28, 2021 that may be closed by this pull request

Provide an option to configure default time zone #410

Closed

lziq self-assigned this Sep 28, 2021

dlurton requested changes Sep 28, 2021

View reviewed changes

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

Changed new field type from Environment to EvaluationSession for `Exp…

27101f4

…rValue.cast` method.

alancai98 requested changes Sep 28, 2021

View reviewed changes

lziq and others added 3 commits September 28, 2021 16:58

Update lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt

3353c93

Co-authored-by: Alan Cai <caialan@amazon.com>

Update lang/test/org/partiql/lang/eval/EvaluatingCompilerDateTimeTest…

5882c2e

…s.kt Co-authored-by: Alan Cai <caialan@amazon.com>

Removed unused package import

9748e44

lziq added 3 commits September 28, 2021 17:12

Provided kdoc for defaultTimezoneOffset in EvaluationSession and …

ff122ee

…`session` in `ExprValue.cast`.

Made changes to settingDefaultTimezoneOffset in `EvaluationSessionT…

919f725

…est`

Merge branch 'configure-default-timezone' of https://github.com/parti…

ba59139

…ql/partiql-lang-kotlin into configure-default-timezone

abhikuhikar reviewed Sep 29, 2021

View reviewed changes

lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt Outdated Show resolved Hide resolved

Created helper function for default timezone offset cast test cases.

f92ee58

abhikuhikar suggested changes Sep 29, 2021

View reviewed changes

lang/test/org/partiql/lang/eval/EvaluatingCompilerDateTimeTests.kt Outdated Show resolved Hide resolved

lang/test/org/partiql/lang/eval/EvaluatingCompilerCastTest.kt Show resolved Hide resolved

Created helper function for default timezone offset time evaluation t…

b7ee8bf

…est cases.

abhikuhikar approved these changes Sep 29, 2021

View reviewed changes

alancai98 approved these changes Sep 29, 2021

View reviewed changes

dlurton approved these changes Sep 29, 2021

View reviewed changes

lziq merged commit 28701e2 into main Sep 29, 2021

lziq deleted the configure-default-timezone branch September 29, 2021 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure default timezone #449

Configure default timezone #449

lziq commented Sep 28, 2021 •

edited

Loading

codecov-commenter commented Sep 28, 2021 •

edited

Loading

dlurton left a comment

alancai98 left a comment

abhikuhikar left a comment

alancai98 left a comment

Configure default timezone #449

Configure default timezone #449

Conversation

lziq commented Sep 28, 2021 • edited Loading

Solution Description:

Changes Details:

Source code:

Test:

codecov-commenter commented Sep 28, 2021 • edited Loading

Codecov Report

dlurton left a comment

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

abhikuhikar left a comment

Choose a reason for hiding this comment

alancai98 left a comment

Choose a reason for hiding this comment

lziq commented Sep 28, 2021 •

edited

Loading

codecov-commenter commented Sep 28, 2021 •

edited

Loading