feat(frontend): cast/cmp with session timezone #7243

jon-chuang · 2023-01-06T09:21:26Z

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

What's changed and what's your intention?

Support:

Cast
- date to timestamptz
- timestamp to timestamptz
- timestamptz to date
- timestamptz to timestamp
- timestamptz to time
Comparison
- date with timestamptz
- timestamp with timestamptz
Cast
- timestamptz to string
  - ~~We have implemented this to always use UTC because it is the basis of many things.~~
  - Now we use the session timezone
- string-without-zone to timestamptz (e.g. '2022-10-01 12:00:00'::timestamptz)

Missing:

Arithmetic
- interval-var + timestamptz = timestamptz (e.g. interval '1' day + '2022-03-13 09:00:00Z'::timestamptz)
- timestamptz + interval-var = timestamptz
- timestamptz - interval-var = timestamptz

When doing so, we will emit the PgResponse NOTICE:

dev=> select '2022-01-01 00:00:00'::timestamp::timestamp with time zone;
NOTICE:  Your session timezone is UTC. It was used in the interpretation of timestamps and dates in your query. If this is unintended,
                change your timezone to match that of your data's with `set timezone = [timezone]` or 
                rewrite your query with an explicit timezone conversion, e.g. with `AT TIME ZONE`.

         timestamp         
---------------------------
 2022-01-01 00:00:00+00:00
(1 row)

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

Refer to a related PR or issue link (optional)

#5826

…on_timezone

…into jon/implicit_cast_with_session_timezone

jon-chuang · 2023-01-06T09:52:39Z

Not sure if this approach is the best.

Preferably, the timezone can become a context that the expression framework can reference rather than embedding in an AT TIME ZONE expression.

For instance, it's hard to support string to timestamptz casting fully, and also implicit casting does not seem to be caught by the binder.

xiangjinwu · 2023-01-09T07:11:27Z

All casts (explicit, assign, implicit) can be handled at a single place: FunctionCall::new_cast. However, it does not have access to self: Binder and has a lot of indirect callers if we want to add a new parameter to it.

As mentioned above, including timezone as context in the expression framework has the advantage of supporting more expressions. It also takes a large effort to refactor, and cannot provide the notice at binding phase. To get a notice when time zone is used implicitly, we still need to (1) update all indirect callers of FunctionCall::new_cast as above; or (2) add an extra frontend phase that visit all exprs. The latter one is easier to extend and maintain, but is more expensive.

Just sharing some thoughts. I do not have a preference over any one.

…already align types

jon-chuang · 2023-01-10T08:28:14Z

Seems the session information including session timezone will be lost during recovery test, as it seems the session is restarted from scratch. Any ideas @yezizp2012?

Perhaps we should remove the .slt file requiring session config from recovery test? (we may need another way to persist the default session config for the database/user)

yezizp2012 · 2023-01-10T08:40:48Z

Seems the session information including session timezone will be lost during recovery test, as it seems the session is restarted from scratch. Any ideas @yezizp2012?

Yes, the frontend will be killed randomly during the recovery test, which will lead the session restart and all config will be reset.

Perhaps we should remove the .slt file requiring session config from recovery test? (we may need another way to persist the default session config for the database/user)

Yes, .slt requiring session config can't be well test by recovery test, we'd better ignore it for now. You can ignore the new slt in this PR here:

risingwave/src/tests/simulation/src/slt.rs

Line 47 in c8323d5

    
           if kill && (path.ends_with("tpch_snapshot.slt") || path.ends_with("tpch_upstream.slt")) {

…into jon/implicit_cast_with_session_timezone

codecov · 2023-01-10T15:31:34Z

Codecov Report

Merging #7243 (334bfd8) into main (47e7b00) will decrease coverage by 0.03%.
The diff coverage is 51.73%.

@@            Coverage Diff             @@
##             main    #7243      +/-   ##
==========================================
- Coverage   73.05%   73.01%   -0.04%     
==========================================
  Files        1068     1069       +1     
  Lines      170827   171099     +272     
==========================================
+ Hits       124799   124935     +136     
- Misses      46028    46164     +136

Flag	Coverage Δ
rust	`73.01% <51.73%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
src/expr/src/expr/build_expr_from_prost.rs	`41.80% <0.00%> (-0.94%)`	⬇️
src/expr/src/expr/expr_binary_nonnull.rs	`52.61% <0.00%> (-3.45%)`	⬇️
src/frontend/src/expr/mod.rs	`80.08% <ø> (ø)`
src/frontend/src/handler/query.rs	`18.78% <0.00%> (-0.43%)`	⬇️
...frontend/src/optimizer/plan_node/batch_hash_agg.rs	`66.07% <0.00%> (ø)`
...ntend/src/optimizer/plan_node/batch_lookup_join.rs	`54.82% <0.00%> (-2.94%)`	⬇️
.../src/optimizer/plan_node/batch_nested_loop_join.rs	`78.72% <0.00%> (-4.43%)`	⬇️
...ontend/src/optimizer/plan_node/batch_simple_agg.rs	`78.57% <0.00%> (ø)`
...frontend/src/optimizer/plan_node/batch_sort_agg.rs	`72.09% <0.00%> (-4.46%)`	⬇️
...c/frontend/src/optimizer/plan_node/batch_update.rs	`60.41% <0.00%> (-7.03%)`	⬇️
... and 36 more

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

fuyufjh

LGTM. Clever solution!

fuyufjh · 2023-01-11T09:31:45Z

src/expr/src/vector_op/timestamptz.rs

@@ -62,6 +65,39 @@ pub fn timestamp_at_time_zone(input: NaiveDateTimeWrapper, time_zone: &str) -> R
    Ok(usec)
 }

+pub fn timestamptz_to_string(elem: i64, time_zone: &str, writer: &mut dyn Write) -> Result<()> {
+    let time_zone = lookup_time_zone(time_zone)?;


So the time_zone string will be looked up for many times. Is there any way to optimize it? Because in most cases this will just a literal. (We can even reject the non-literal cases, I think)

The lookup is a hash table lookup, with hashing cost linear to string length. The bigger cost is actually our way of evaluating literal expr, which would create a column full of duplicated values.
The ideal case, of course, is to pass a single session timezone as task context from frontend to compute node, and do a single lookup. But I guess we can defer that refactor until there is a more useful case for expr eval context.

It does seem reasonable if the timezone string is replaced by an i32 fixed offset that is looked up by front-end... cloning i32 array is probably much cheaper than string array.

But it is not a fixed offset. The offset can vary at different dates.

Hmm, indeed. We could represent it by the chrono_tz enum value, and repr in our type system as i32?

https://docs.rs/chrono-tz/latest/chrono_tz/static.TZ_VARIANTS.html#

It may be a bit brittle. But an i32 at the RHS of AT TIME ZONE and CAST_WITH_TIMEZONE will always be interpreted as a chrono_tz timezone.

I think the binder should not look it up. We can peform the lookup at the same place as looking up session_timezone. That way, it will still be human readable until it is turned into a proto plan.

Btw, we don't support strings such as UTC+8 via chrono_tz. Do you think we should support these fixed offsets as well? We can reserve some space in our i32 encoding for such strings, and look them up separately.

created issue: #7335

src/expr/src/expr/expr_binary_nonnull.rs

src/expr/src/vector_op/timestamptz.rs

e2e_test/streaming/dynamic_filter.slt

src/expr/src/vector_op/timestamptz.rs

src/frontend/src/expr/mod.rs

xiangjinwu · 2023-01-11T11:59:31Z

src/frontend/src/expr/mod.rs

+                let input_type = input.return_type();
+                match (input_type.clone(), return_type.clone()) {
+                    (DataType::Timestamptz, DataType::Varchar)
+                    | (DataType::Varchar, DataType::Timestamptz) => {


Just to make it clear (rather than to suggest any actions), this may be overly noisy even when all input strings already contains timezone (2022-01-01 12:00:00-08:00) and the session timezone is actually unused. It is impossible to know this in frontend but only during execution.

As a special case, when the input is a literal string, we can do the parsing in frontend and avoid casting expr at all. This would improve the user experience. Opened #7320 to track it.

chenzl25

LGTM! Great!

chenzl25 · 2023-01-11T13:48:45Z

e2e_test/batch/functions/session_timezone.slt

+statement ok
+set timezone = "us/pacific";
+
+# Cast date to timestamptz
+query T
+select '2022-01-01'::date::timestamp with time zone;
+----
+2022-01-01 08:00:00+00:00
+
+# Cast timestamp to timestamptz
+query T
+select '2022-01-01 00:00:00'::timestamp::timestamp with time zone;
+----
+2022-01-01 08:00:00+00:00


We should display 2022-01-01 00:00:00-08:00 or 2022-01-01 08:00:00+00:00? Although they are equal.

Yes, this will be in an upcoming PR (depending on this PR). It will format Pg rows with the session timestamp.

github-actions

license-eye has totally checked 2644 files.

Valid	Invalid	Ignored	Fixed
1258	1	1385	0

Click to see the invalid file list

src/frontend/src/expr/session_timezone.rs

src/frontend/src/expr/session_timezone.rs

xiangjinwu

rest lgtm

src/expr/src/vector_op/timestamptz.rs

jon-chuang added 12 commits January 5, 2023 19:20

commit

3f004a8

stash

ebe08ef

fix

99aac3a

Merge branch 'jon/session_timezone' into jon/implicit_cast_with_sessi…

e97d28a

…on_timezone

stash

6e14642

revert default

5dd7e9c

Merge branch 'jon/session_timezone' into jon/implicit_cast_with_sessi…

d42173e

…on_timezone

stash

e51dac8

Merge branch 'main' of https://github.com/singularity-data/risingwave …

b9561bb

…into jon/implicit_cast_with_session_timezone

commit

a6ff87e

minor

1aa991d

minor

eef6e07

github-actions bot added the type/feature label Jan 6, 2023

minor

f18d7b4

jon-chuang requested a review from xiangjinwu January 6, 2023 09:25

minor

11f68cf

jon-chuang changed the title ~~feat(frontend): cast/cmp with session timezone~~ [draft] feat(frontend): cast/cmp with session timezone Jan 6, 2023

github-actions bot added Invalid PR Title and removed type/feature labels Jan 6, 2023

BugenZhao mentioned this pull request Jan 6, 2023

bug: create mv with now() affects the outcome of select the base table #7247

Closed

jon-chuang added 8 commits January 9, 2023 17:29

stash

560b35f

Merge branch 'main' into jon/implicit_cast_with_session_timezone

d57822c

undo

533288d

changes

768af61

for all

b1e23e0

add dynamic filter test with timestamptz/timestamp - redundant as we …

5e8207b

…already align types

add

637b86b

fix

7a2bf8e

github-actions bot added the type/feature label Jan 10, 2023

revert to UTC at end of test

a16d522

jon-chuang added 4 commits January 10, 2023 16:54

fix kafka

031ed37

planner tests, ignore slt

b9ef349

Merge branch 'main' of https://github.com/singularity-data/risingwave …

ceb80b3

…into jon/implicit_cast_with_session_timezone

minor

af2b99f

minor

cffef1b

fuyufjh approved these changes Jan 11, 2023

View reviewed changes

fuyufjh requested review from chenzl25 and yezizp2012 January 11, 2023 09:40

fuyufjh reviewed Jan 11, 2023

View reviewed changes

src/expr/src/vector_op/timestamptz.rs Outdated Show resolved Hide resolved

xiangjinwu reviewed Jan 11, 2023

View reviewed changes

chenzl25 approved these changes Jan 11, 2023

View reviewed changes

code review

31b8a78

github-actions bot reviewed Jan 12, 2023

View reviewed changes

src/frontend/src/expr/session_timezone.rs Show resolved Hide resolved

jon-chuang added 2 commits January 12, 2023 10:41

minor

e054d54

add license

05a605e

This was referenced Jan 12, 2023

Tracking: Session Timezone #7334

Closed

discuss(optimization): encode timezone strings as i32 #7335

Open

minor

bb03a47

xiangjinwu approved these changes Jan 12, 2023

View reviewed changes

src/expr/src/vector_op/timestamptz.rs Outdated Show resolved Hide resolved

remove

f9ec0be

jon-chuang added the mergify/can-merge label Jan 12, 2023

Merge branch 'main' into jon/implicit_cast_with_session_timezone

334bfd8

mergify bot merged commit eca9541 into main Jan 12, 2023

mergify bot deleted the jon/implicit_cast_with_session_timezone branch January 12, 2023 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): cast/cmp with session timezone #7243

feat(frontend): cast/cmp with session timezone #7243

jon-chuang commented Jan 6, 2023 •

edited

Loading

jon-chuang commented Jan 6, 2023

xiangjinwu commented Jan 9, 2023

jon-chuang commented Jan 10, 2023 •

edited

Loading

yezizp2012 commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

fuyufjh left a comment

fuyufjh Jan 11, 2023

xiangjinwu Jan 11, 2023

jon-chuang Jan 11, 2023 •

edited

Loading

xiangjinwu Jan 11, 2023

jon-chuang Jan 12, 2023 •

edited

Loading

jon-chuang Jan 12, 2023 •

edited

Loading

jon-chuang Jan 12, 2023

xiangjinwu Jan 11, 2023 •

edited

Loading

chenzl25 left a comment

chenzl25 Jan 11, 2023

jon-chuang Jan 12, 2023 •

edited

Loading

github-actions bot left a comment

xiangjinwu left a comment

feat(frontend): cast/cmp with session timezone #7243

feat(frontend): cast/cmp with session timezone #7243

Conversation

jon-chuang commented Jan 6, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Refer to a related PR or issue link (optional)

jon-chuang commented Jan 6, 2023

xiangjinwu commented Jan 9, 2023

jon-chuang commented Jan 10, 2023 • edited Loading

yezizp2012 commented Jan 10, 2023 • edited Loading

codecov bot commented Jan 10, 2023 • edited Loading

Codecov Report

fuyufjh left a comment

Choose a reason for hiding this comment

fuyufjh Jan 11, 2023

Choose a reason for hiding this comment

xiangjinwu Jan 11, 2023

Choose a reason for hiding this comment

jon-chuang Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

xiangjinwu Jan 11, 2023

Choose a reason for hiding this comment

jon-chuang Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

jon-chuang Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

jon-chuang Jan 12, 2023

Choose a reason for hiding this comment

xiangjinwu Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

chenzl25 Jan 11, 2023

Choose a reason for hiding this comment

jon-chuang Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

xiangjinwu left a comment

Choose a reason for hiding this comment

jon-chuang commented Jan 6, 2023 •

edited

Loading

jon-chuang commented Jan 10, 2023 •

edited

Loading

yezizp2012 commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

jon-chuang Jan 11, 2023 •

edited

Loading

jon-chuang Jan 12, 2023 •

edited

Loading

jon-chuang Jan 12, 2023 •

edited

Loading

xiangjinwu Jan 11, 2023 •

edited

Loading

jon-chuang Jan 12, 2023 •

edited

Loading