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(types): introduce Timestamptz type #10477

Merged
merged 7 commits into from
Jul 5, 2023
Merged

feat(types): introduce Timestamptz type #10477

merged 7 commits into from
Jul 5, 2023

Conversation

wangrunji0408
Copy link
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR introduces Timestamptz type and array. It's another attempt after #7084.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

Documentation

  • My PR contains user-facing changes.

@BugenZhao
Copy link
Member

BugenZhao commented Jun 28, 2023

May I ask whether we're going to unify logical types (DataType) and physical types (ScalarImpl::..) after this PR?


Seems still impossible since there could some metadata in DataType like the field name of structs. But we can now establish 1v1 relationship from logical types to physical types.

@BugenZhao BugenZhao self-requested a review June 28, 2023 07:23
@wangrunji0408
Copy link
Contributor Author

wangrunji0408 commented Jun 28, 2023

May I ask whether we're going to unify logical types (DataType) and physical types (ScalarImpl::..) after this PR?

Seems still impossible since there could some metadata in DataType like the field name of structs. But we can now establish 1v1 relationship from logical types to physical types.

Exactly. Even though we have a 1-1 mapping between the variants of DataType and ScalarImpl (which is good), for some types it still depends on the metadata in DataType to interpret values in ScalarImpl.

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Jul 3, 2023

One example is ScalarImpl::List with a ListValue containing [] or [None]. It is still necessary to have DataType to know whether it is int[] or varchar[] or int[][].

@xiangjinwu
Copy link
Contributor

Oh sorry the build failure is due to an uncaught conflict with #10462. Should be an easy fix. I am reviewing the other parts

Signed-off-by: Runji Wang <wangrunji0408@163.com>
Copy link
Contributor

@xiangjinwu xiangjinwu left a comment

Choose a reason for hiding this comment

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

Rest lgtm. Thank you!

src/common/src/types/timestamptz.rs Outdated Show resolved Hide resolved
src/common/src/array/arrow.rs Outdated Show resolved Hide resolved
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
@wangrunji0408 wangrunji0408 added this pull request to the merge queue Jul 5, 2023
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #10477 (fe3ec24) into main (296348a) will decrease coverage by 0.01%.
The diff coverage is 65.37%.

@@            Coverage Diff             @@
##             main   #10477      +/-   ##
==========================================
- Coverage   70.16%   70.15%   -0.01%     
==========================================
  Files        1295     1296       +1     
  Lines      221572   221690     +118     
==========================================
+ Hits       155455   155527      +72     
- Misses      66117    66163      +46     
Flag Coverage Δ
rust 70.15% <65.37%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/array/arrow.rs 67.00% <0.00%> (-1.72%) ⬇️
src/common/src/array/chrono_array.rs 100.00% <ø> (ø)
src/common/src/array/proto_reader.rs 87.17% <0.00%> (-1.71%) ⬇️
src/common/src/array/stream_chunk.rs 80.96% <ø> (ø)
src/common/src/hash/key.rs 80.67% <0.00%> (-1.19%) ⬇️
src/common/src/test_utils/rand_chunk.rs 79.16% <0.00%> (ø)
src/common/src/types/datetime.rs 62.74% <ø> (+0.72%) ⬆️
src/common/src/types/to_text.rs 74.52% <ø> (-2.34%) ⬇️
src/connector/src/parser/csv_parser.rs 87.09% <0.00%> (ø)
src/expr/macro/src/lib.rs 94.73% <ø> (ø)
... and 30 more

... and 2 files with indirect coverage changes

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

Merged via the queue into main with commit 005f249 Jul 5, 2023
35 of 36 checks passed
@wangrunji0408 wangrunji0408 deleted the wrj/timestamptz branch July 5, 2023 04:48
wangrunji0408 added a commit that referenced this pull request Jul 11, 2023
Signed-off-by: Runji Wang <wangrunji0408@163.com>
lmatz added a commit that referenced this pull request Jul 11, 2023
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Co-authored-by: lmatz <lmatz823@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants