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.

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

refactor(common): cleanup unused methods on IntervalUnit #8456

Merged
merged 4 commits into from
Mar 9, 2023
Merged

refactor(common): cleanup unused methods on IntervalUnit #8456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f76896
cleanup stale IntervalUnit::from_protobuf_bytes and to_protobuf_owned
xiangjinwu Mar 9, 2023
74e26c0
cleanup confusing get_years
xiangjinwu Mar 9, 2023
a445d26
deprecate as_ms_i64 and from_total_ms
xiangjinwu Mar 9, 2023
fb8de6a
doc comments
xiangjinwu Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/common/src/array/interval_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,16 @@ mod tests {
}
let ret_arr = array_builder.finish();
for v in ret_arr.iter().flatten() {
assert_eq!(v.get_years(), 1);
assert_eq!(v.get_months(), 12);
assert_eq!(v.get_days(), 0);
}
let ret_arr = IntervalArray::from_iter([Some(IntervalUnit::from_ymd(1, 0, 0)), None]);
let v = ret_arr.value_at(0).unwrap();
assert_eq!(v.get_years(), 1);
assert_eq!(v.get_months(), 12);
assert_eq!(v.get_days(), 0);
Comment on lines 40 to 43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the test itself shows, 1 year 0 month 0 day is returning

        assert_eq!(v.get_years(), 1);
        assert_eq!(v.get_months(), 12);
        assert_eq!(v.get_days(), 0);

which is inconsistent and confusing.

let v = ret_arr.value_at(1);
assert_eq!(v, None);
let v = unsafe { ret_arr.value_at_unchecked(0).unwrap() };
assert_eq!(v.get_years(), 1);
assert_eq!(v.get_months(), 12);
assert_eq!(v.get_days(), 0);
}
Expand Down
62 changes: 14 additions & 48 deletions src/common/src/types/interval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use std::hash::{Hash, Hasher};
use std::io::Write;
use std::ops::{Add, Neg, Sub};

use anyhow::anyhow;
use byteorder::{BigEndian, NetworkEndian, ReadBytesExt, WriteBytesExt};
use bytes::BytesMut;
use num_traits::{CheckedAdd, CheckedNeg, CheckedSub, Zero};
Expand Down Expand Up @@ -69,10 +68,6 @@ impl IntervalUnit {
self.months
}

pub fn get_years(&self) -> i32 {
self.months / 12
}

pub fn get_ms(&self) -> i64 {
self.ms
}
Expand All @@ -81,40 +76,12 @@ impl IntervalUnit {
self.ms.rem_euclid(DAY_MS) as u64
}

pub fn from_protobuf_bytes(bytes: &[u8], ty: IntervalType) -> ArrayResult<Self> {
// TODO: remove IntervalType later.
match ty {
// the unit is months
Year | YearToMonth | Month => {
let bytes = bytes
.try_into()
.map_err(|e| anyhow!("Failed to deserialize i32: {:?}", e))?;
let mouths = i32::from_be_bytes(bytes);
Ok(IntervalUnit::from_month(mouths))
}
// the unit is ms
Day | DayToHour | DayToMinute | DayToSecond | Hour | HourToMinute | HourToSecond
| Minute | MinuteToSecond | Second => {
let bytes = bytes
.try_into()
.map_err(|e| anyhow!("Failed to deserialize i64: {:?}", e))?;
let ms = i64::from_be_bytes(bytes);
Ok(IntervalUnit::from_millis(ms))
}
Unspecified => {
// Invalid means the interval is from the new frontend.
// TODO: make this default path later.
let mut cursor = Cursor::new(bytes);
read_interval_unit(&mut cursor)
}
}
}

/// Justify interval, convert 1 month to 30 days and 86400 ms to 1 day.
/// If day is positive, complement the ms negative value.
/// These rules only use in interval comparison.
pub fn justify_interval(&mut self) {
let total_ms = self.total_ms();
#[expect(deprecated)]
let total_ms = self.as_ms_i64();
*self = Self {
months: 0,
days: (total_ms / DAY_MS) as i32,
Expand All @@ -128,8 +95,8 @@ impl IntervalUnit {
interval
}

#[must_use]
pub fn from_total_ms(ms: i64) -> Self {
#[deprecated]
fn from_total_ms(ms: i64) -> Self {
let mut remaining_ms = ms;
let months = remaining_ms / MONTH_MS;
remaining_ms -= months * MONTH_MS;
Expand All @@ -142,10 +109,6 @@ impl IntervalUnit {
}
}

pub fn total_ms(&self) -> i64 {
self.months as i64 * MONTH_MS + self.days as i64 * DAY_MS + self.ms
}

#[must_use]
pub fn from_ymd(year: i32, month: i32, days: i32) -> Self {
let months = year * 12 + month;
Expand Down Expand Up @@ -186,13 +149,6 @@ impl IntervalUnit {
}
}

pub fn to_protobuf_owned(self) -> Vec<u8> {
let buf = BytesMut::with_capacity(16);
let mut writer = buf.writer();
self.to_protobuf(&mut writer).unwrap();
writer.into_inner().to_vec()
}

pub fn to_protobuf<T: Write>(self, output: &mut T) -> ArrayResult<usize> {
output.write_i32::<BigEndian>(self.months)?;
output.write_i32::<BigEndian>(self.days)?;
Expand Down Expand Up @@ -225,10 +181,13 @@ impl IntervalUnit {
return None;
}

#[expect(deprecated)]
let ms = self.as_ms_i64();
#[expect(deprecated)]
Some(IntervalUnit::from_total_ms((ms as f64 / rhs).round() as i64))
}

#[deprecated]
fn as_ms_i64(&self) -> i64 {
self.months as i64 * MONTH_MS + self.days as i64 * DAY_MS + self.ms
}
Expand All @@ -241,7 +200,9 @@ impl IntervalUnit {
let rhs = rhs.try_into().ok()?;
let rhs = rhs.0;

#[expect(deprecated)]
let ms = self.as_ms_i64();
#[expect(deprecated)]
Some(IntervalUnit::from_total_ms((ms as f64 * rhs).round() as i64))
}

Expand Down Expand Up @@ -474,6 +435,8 @@ impl IntervalUnit {
}
}

/// Wrapper so that `Debug for IntervalUnitDisplay` would use the concise format of `Display for
/// IntervalUnit`.
#[derive(Clone, Copy)]
pub struct IntervalUnitDisplay<'a> {
pub core: &'a IntervalUnit,
Expand All @@ -491,6 +454,8 @@ impl std::fmt::Debug for IntervalUnitDisplay<'_> {
}
}

/// Loss of information during the process due to `justify`. Only intended for memcomparable
/// encoding.
impl Serialize for IntervalUnit {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
Expand All @@ -512,6 +477,7 @@ impl<'de> Deserialize<'de> for IntervalUnit {
}
}

/// Duplicated logic only used by `HopWindow`. See #8452.
#[expect(clippy::from_over_into)]
impl Into<IntervalUnitProto> for IntervalUnit {
fn into(self) -> IntervalUnitProto {
Expand Down
8 changes: 3 additions & 5 deletions src/common/src/types/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@ mod scalar_impl;
mod successor;

use std::fmt::Debug;
use std::io::Cursor;
use std::str::{FromStr, Utf8Error};

pub use native_type::*;
use risingwave_pb::data::data_type::IntervalType::*;
use risingwave_pb::data::data_type::{IntervalType, TypeName};
use risingwave_pb::data::data_type::TypeName;
pub use scalar_impl::*;
pub use successor::*;
pub mod chrono_wrapper;
Expand Down Expand Up @@ -68,8 +66,8 @@ use self::to_binary::ToBinary;
use self::to_text::ToText;
use crate::array::serial_array::Serial;
use crate::array::{
read_interval_unit, ArrayBuilderImpl, JsonbRef, JsonbVal, ListRef, ListValue,
PrimitiveArrayItemType, StructRef, StructValue,
ArrayBuilderImpl, JsonbRef, JsonbVal, ListRef, ListValue, PrimitiveArrayItemType, StructRef,
StructValue,
};
use crate::error::Result as RwResult;

Expand Down