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

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

Open
xiangjinwu opened this issue Mar 9, 2023 · 1 comment
Open

Comments

@xiangjinwu
Copy link
Contributor

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

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;
}

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:

#[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,
}
}
}

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.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant