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): support ns timestamp #19827

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

xxhZs
Copy link
Contributor

@xxhZs xxhZs commented Dec 17, 2024

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

What's changed and what's your intention?

our timestamp only support ms, But our internal implementation is capable of ns, it's just that we automatically convert it to ms when we read it in, and this pr can read ns to support ns.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

@xxhZs xxhZs requested a review from a team as a code owner December 17, 2024 08:37
@xxhZs xxhZs requested a review from xxchan December 17, 2024 08:37
@graphite-app graphite-app bot requested a review from a team December 17, 2024 09:25
@hzxa21 hzxa21 requested a review from wcy-fdu December 18, 2024 03:40
@wcy-fdu
Copy link
Contributor

wcy-fdu commented Dec 18, 2024

I tested whether I could read nanosecond in the parquet file on this branch, and it seemed to work.
image

@wcy-fdu wcy-fdu enabled auto-merge December 19, 2024 04:39
@wcy-fdu wcy-fdu disabled auto-merge December 19, 2024 04:40
@wcy-fdu
Copy link
Contributor

wcy-fdu commented Dec 19, 2024

sorry, mis clicked update branch

@wcy-fdu
Copy link
Contributor

wcy-fdu commented Dec 19, 2024

cc @xiangjinwu PTAL❤️

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.

  • Could you also include the testing SQLs you have tried?
  • Is any caller of to_protobuf/from_protobuf assuming that Timestamp would be fixed 64-bit? This assumption is no longer true.

)
}
let dt = s
.parse::<jiff::civil::DateTime>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to introduce yet another dependency in addition to chrono and speedate?

@@ -495,18 +491,47 @@ impl Timestamp {
}

pub fn from_protobuf(cur: &mut Cursor<&[u8]>) -> ArrayResult<Timestamp> {
let micros = cur
let secs = cur
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: secs_or_usecs

Comment on lines +524 to +534
fn add_timestamp_namo_format_state(value: i64) -> i64 {
value ^ (0b01 << 62)
}

fn has_timestamp_namo_format_state(value: i64) -> bool {
let state = (value >> 62) & 0b11;
state == 0b10 || state == 0b01
}

fn remove_timestamp_namo_format_state(value: i64) -> i64 {
value ^ (0b01 << 62)
Copy link
Contributor

Choose a reason for hiding this comment

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

Encapsulate as a new type:

/// document about old and new format, including the meaning of highest 2 bits, and the corresponding accepted ranges.
enum FirstI64 {
  V0{ usecs: i64 }
  V1{ secs: i64 }
}
impl From<i64> for FirstI64 {
  // check and remove the tagging bits
}
impl From<FirstI64> for i64 {
  // add the tagging bits
}

@@ -495,7 +495,7 @@ impl HummockVersion {
.clone_from(&group_construct.table_ids);
self.levels.insert(*compaction_group_id, new_levels);
let member_table_ids = if group_construct.version
>= CompatibilityVersion::NoMemberTableIds as _
>= CompatibilityVersion::NoMemberTableIds as i32
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid unrelated change

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.

4 participants