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

feat(expr): AT TIME ZONE to convert between timestamp and timestamptz #5968

Merged
merged 8 commits into from
Oct 25, 2022

Conversation

xiangjinwu
Copy link
Contributor

@xiangjinwu xiangjinwu commented Oct 21, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Casting between timestamp and timestamp with time zone requires an implicit session TimeZone, which is not supported yet (#5826).

The AT TIME ZONE operator is another way to bridge these two types, via a TimeZone parameter explicitly specified in the query. This PR implements this expr.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

Types of user-facing changes

  • SQL commands, functions, and operators

Release note

  • timestamp_value AT TIME ZONE 'zone' -> timestamp_with_time_zone_value
  • timestamp_with_time_zone_value AT TIME ZONE 'zone' -> timestamp_value

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #5968 (9024ca1) into main (d72ba12) will increase coverage by 0.10%.
The diff coverage is 88.79%.

@@            Coverage Diff             @@
##             main    #5968      +/-   ##
==========================================
+ Coverage   74.57%   74.68%   +0.10%     
==========================================
  Files         924      928       +4     
  Lines      147420   147871     +451     
==========================================
+ Hits       109945   110438     +493     
+ Misses      37475    37433      -42     
Flag Coverage Δ
rust 74.68% <88.79%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
src/ctl/src/cmd_impl/hummock/list_kv.rs 0.00% <0.00%> (ø)
src/ctl/src/cmd_impl/hummock/sst_dump.rs 0.00% <0.00%> (ø)
src/expr/src/expr/expr_binary_nonnull.rs 71.69% <0.00%> (-5.98%) ⬇️
src/frontend/src/binder/expr/mod.rs 79.88% <0.00%> (-1.83%) ⬇️
src/frontend/src/handler/query.rs 19.52% <0.00%> (-0.72%) ⬇️
src/frontend/src/optimizer/plan_node/mod.rs 84.03% <ø> (ø)
src/frontend/src/optimizer/plan_node/stream.rs 0.00% <0.00%> (ø)
...rontend/src/scheduler/distributed/query_manager.rs 15.21% <0.00%> (-0.12%) ⬇️
src/meta/src/stream/source_manager.rs 45.65% <0.00%> (+0.44%) ⬆️
src/source/src/lib.rs 90.00% <ø> (ø)
... and 112 more

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

@xiangjinwu xiangjinwu marked this pull request as ready for review October 21, 2022 10:05

#[inline(always)]
pub fn timestamp_at_time_zone(input: NaiveDateTimeWrapper, time_zone: &str) -> Result<i64> {
let time_zone = parse_time_zone(time_zone)?;
Copy link
Member

Choose a reason for hiding this comment

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

Seems we need to parse the timezone every row, not sure whether it can be heavy. 🤔

Some possible optimizations:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parsing is a lookup of a static and perfect hash table (crate phf), where hash is done in a case-insensitive manner. So memoization / cache has no advantage here.

It is still true that most cases this is a constant, though.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Then there should be no problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the function name to lookup_time_zone.

Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify mergify bot merged commit 759b374 into main Oct 25, 2022
@mergify mergify bot deleted the XJ-feat-tstz-at-time-zone branch October 25, 2022 10:50
@xiangjinwu xiangjinwu added the user-facing-changes Contains changes that are visible to users label Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants