8000 refactor(common): consolidate `interval` protobuf encode/decode logic · Issue #8452 · risingwavelabs/risingwave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

refactor(common): consolidate interval protobuf encode/decode logic #8452

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.

Sign up for GitHub

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

Jump to bottom
Closed
xiangjinwu opened this issue Mar 9, 2023 · 1 comment
Closed

refactor(common): consolidate interval protobuf encode/decode logic #8452

xiangjinwu opened this issue Mar 9, 2023 · 1 comment
Assignees
@xiangjinwu
Labels
breaking-change no-issue-activity type/refactor Type: Refactoring.
Milestone
release-2.3

Comments

@xiangjinwu
Copy link
Contributor

As with other PrimitiveArrayItemType, an interval value is encoded to and decoded from protobuf in the following:

risingwave/src/common/src/types/interval.rs

Lines 196 to 201 in 630685f

pub fn to_protobuf<T: Write>(self, output: &mut T) -> ArrayResult<usize> {
output.write_i32::<BigEndian>(self.months)?;
output.write_i32::<BigEndian>(self.days)?;
output.write_i64::<BigEndian>(self.ms)?;
Ok(16)
}

risingwave/proto/data.proto

Lines 111 to 117 in 630685f

message Datum {
// bool array/bitmap: one byte, 0 for false (null), non-zero for true (non-null)
// integer, float, double: big-endianness
// interval: encoded to (months, days, milliseconds), big-endianness
// varchar: encoded accorded to encoding, currently only utf8 is supported.
bytes body = 1;
}

risingwave/src/common/src/array/column_proto_readers.rs

Lines 100 to 113 in 953e4b2

pub fn read_interval_unit(cursor: &mut Cursor<&[u8]>) -> ArrayResult<IntervalUnit> {
let mut read = || {
let months = cursor.read_i32::<BigEndian>()?;
let days = cursor.read_i32::<BigEndian>()?;
let ms = cursor.read_i64::<BigEndian>()?;
Ok::<_, std::io::Error>(IntervalUnit::new(months, days, ms))
};
match read() {
Ok(iu) => Ok(iu),
Err(e) => bail!("Failed to read IntervalUnit from buffer: {}", e),
}
}

However, as a special case, when it is part of HopWindow, there exists another code path:

risingwave/src/common/src/types/interval.rs

Lines 515 to 534 in 630685f

#[expect(clippy::from_over_into)]
impl Into<IntervalUnitProto> for IntervalUnit {
fn into(self) -> IntervalUnitProto {
IntervalUnitProto {
months: self.months,
days: self.days,
ms: self.ms,
}
}
}
impl From<&'_ IntervalUnitProto> for IntervalUnit {
fn from(p: &'_ IntervalUnitProto) -> Self {
Self {
months: p.months,
days: p.days,
ms: p.ms,
}
}
}

risingwave/proto/data.proto

Lines 10 to 14 in 630685f

message IntervalUnit {
int32 months = 1;
int32 days = 2;
int64 ms = 3;
}

It is better to consolidate them, although we do not foresee many future changes except #4514 and the maintenance cost of keeping them in sync may not be high.
@xiangjinwu xiangjinwu added the type/refactor Type: Refactoring. label Mar 9, 2023
@github-actions github-actions bot added this to the release-0.1.18 milestone Mar 9, 2023
This was referenced Mar 9, 2023
Merged
Merged
@xiangjinwu xiangjinwu modified the milestones: release-0.18, release-0.19 Mar 22, 2023
@xiangjinwu xiangjinwu removed this from the release-0.19 milestone Apr 4, 2023
@github-actions
Copy link
Contributor
github-actions bot commented Jun 4, 2023

This issue has been open for 60 days with no activity. Could you please update the status? Feel free to continue discussion or close as not planned.

@xiangjinwu xiangjinwu self-assigned this Jan 8, 2025
@xiangjinwu xiangjinwu added this to the release-2.2 milestone Jan 8, 2025
@xiangjinwu xiangjinwu modified the milestones: release-2.2, release-2.3 Feb 12, 2025
@xiangjinwu xiangjinwu closed this as not planned Won't fix, can't repro, duplicate, stale Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xiangjinwu xiangjinwu

Labels
breaking-change no-issue-activity type/refactor Type: Refactoring.
Projects
None yet
Milestone
release-2.3
Development

No branches or pull requests
1 participant
@xiangjinwu
0