From 5dc7ce179153dbc945571cc69cc9d9cf194178e4 Mon Sep 17 00:00:00 2001 From: Wei <15172118655@163.com> Date: Mon, 18 Dec 2023 11:06:11 +0800 Subject: [PATCH] fix: typos and bit operations (#2944) * fix: typos and bit operation * fix: helper * chore: add tests in decimal128.rs and interval.rs * chore: test * chore: change proto version * chore: clippy --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/api/src/helper.rs | 45 ++++++++++------------- src/common/decimal/src/decimal128.rs | 36 +++++++++++++++++-- src/common/time/src/interval.rs | 49 ++++++++++++++++++-------- src/operator/src/req_convert/common.rs | 6 ++-- 6 files changed, 91 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ffb2589a0dde..5aa53779d444 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3589,7 +3589,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=b1d403088f02136bcebde53d604f491c260ca8e2#b1d403088f02136bcebde53d604f491c260ca8e2" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=a31ea166fc015ea7ff111ac94e26c3a5d64364d2#a31ea166fc015ea7ff111ac94e26c3a5d64364d2" dependencies = [ "prost 0.12.2", "serde", diff --git a/Cargo.toml b/Cargo.toml index 06f8475f6a3c..10f5a99a7b31 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -88,7 +88,7 @@ etcd-client = "0.12" fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "b1d403088f02136bcebde53d604f491c260ca8e2" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "a31ea166fc015ea7ff111ac94e26c3a5d64364d2" } humantime-serde = "1.1" itertools = "0.10" lazy_static = "1.4" diff --git a/src/api/src/helper.rs b/src/api/src/helper.rs index 40b8d1533125..ff4911e1a29c 100644 --- a/src/api/src/helper.rs +++ b/src/api/src/helper.rs @@ -535,11 +535,8 @@ pub fn convert_i128_to_interval(v: i128) -> v1::IntervalMonthDayNano { /// Convert common decimal128 to grpc decimal128 without precision and scale. pub fn convert_to_pb_decimal128(v: Decimal128) -> v1::Decimal128 { - let value = v.val(); - v1::Decimal128 { - hi: (value >> 64) as i64, - lo: value as i64, - } + let (hi, lo) = v.split_value(); + v1::Decimal128 { hi, lo } } pub fn pb_value_to_value_ref<'a>( @@ -580,9 +577,9 @@ pub fn pb_value_to_value_ref<'a>( ValueData::TimeMillisecondValue(t) => ValueRef::Time(Time::new_millisecond(*t)), ValueData::TimeMicrosecondValue(t) => ValueRef::Time(Time::new_microsecond(*t)), ValueData::TimeNanosecondValue(t) => ValueRef::Time(Time::new_nanosecond(*t)), - ValueData::IntervalYearMonthValues(v) => ValueRef::Interval(Interval::from_i32(*v)), - ValueData::IntervalDayTimeValues(v) => ValueRef::Interval(Interval::from_i64(*v)), - ValueData::IntervalMonthDayNanoValues(v) => { + ValueData::IntervalYearMonthValue(v) => ValueRef::Interval(Interval::from_i32(*v)), + ValueData::IntervalDayTimeValue(v) => ValueRef::Interval(Interval::from_i64(*v)), + ValueData::IntervalMonthDayNanoValue(v) => { let interval = Interval::from_month_day_nano(v.months, v.days, v.nanoseconds); ValueRef::Interval(interval) } @@ -986,13 +983,13 @@ pub fn to_proto_value(value: Value) -> Option { }, Value::Interval(v) => match v.unit() { IntervalUnit::YearMonth => v1::Value { - value_data: Some(ValueData::IntervalYearMonthValues(v.to_i32())), + value_data: Some(ValueData::IntervalYearMonthValue(v.to_i32())), }, IntervalUnit::DayTime => v1::Value { - value_data: Some(ValueData::IntervalDayTimeValues(v.to_i64())), + value_data: Some(ValueData::IntervalDayTimeValue(v.to_i64())), }, IntervalUnit::MonthDayNano => v1::Value { - value_data: Some(ValueData::IntervalMonthDayNanoValues( + value_data: Some(ValueData::IntervalMonthDayNanoValue( convert_i128_to_interval(v.to_i128()), )), }, @@ -1011,12 +1008,9 @@ pub fn to_proto_value(value: Value) -> Option { value_data: Some(ValueData::DurationNanosecondValue(v.value())), }, }, - Value::Decimal128(v) => { - let (hi, lo) = v.split_value(); - v1::Value { - value_data: Some(ValueData::Decimal128Value(v1::Decimal128 { hi, lo })), - } - } + Value::Decimal128(v) => v1::Value { + value_data: Some(ValueData::Decimal128Value(convert_to_pb_decimal128(v))), + }, Value::List(_) => return None, }; @@ -1051,9 +1045,9 @@ pub fn proto_value_type(value: &v1::Value) -> Option { ValueData::TimeMillisecondValue(_) => ColumnDataType::TimeMillisecond, ValueData::TimeMicrosecondValue(_) => ColumnDataType::TimeMicrosecond, ValueData::TimeNanosecondValue(_) => ColumnDataType::TimeNanosecond, - ValueData::IntervalYearMonthValues(_) => ColumnDataType::IntervalYearMonth, - ValueData::IntervalDayTimeValues(_) => ColumnDataType::IntervalDayTime, - ValueData::IntervalMonthDayNanoValues(_) => ColumnDataType::IntervalMonthDayNano, + ValueData::IntervalYearMonthValue(_) => ColumnDataType::IntervalYearMonth, + ValueData::IntervalDayTimeValue(_) => ColumnDataType::IntervalDayTime, + ValueData::IntervalMonthDayNanoValue(_) => ColumnDataType::IntervalMonthDayNano, ValueData::DurationSecondValue(_) => ColumnDataType::DurationSecond, ValueData::DurationMillisecondValue(_) => ColumnDataType::DurationMillisecond, ValueData::DurationMicrosecondValue(_) => ColumnDataType::DurationMicrosecond, @@ -1109,10 +1103,10 @@ pub fn value_to_grpc_value(value: Value) -> GrpcValue { TimeUnit::Nanosecond => ValueData::TimeNanosecondValue(v.value()), }), Value::Interval(v) => Some(match v.unit() { - IntervalUnit::YearMonth => ValueData::IntervalYearMonthValues(v.to_i32()), - IntervalUnit::DayTime => ValueData::IntervalDayTimeValues(v.to_i64()), + IntervalUnit::YearMonth => ValueData::IntervalYearMonthValue(v.to_i32()), + IntervalUnit::DayTime => ValueData::IntervalDayTimeValue(v.to_i64()), IntervalUnit::MonthDayNano => { - ValueData::IntervalMonthDayNanoValues(convert_i128_to_interval(v.to_i128())) + ValueData::IntervalMonthDayNanoValue(convert_i128_to_interval(v.to_i128())) } }), Value::Duration(v) => Some(match v.unit() { @@ -1121,10 +1115,7 @@ pub fn value_to_grpc_value(value: Value) -> GrpcValue { TimeUnit::Microsecond => ValueData::DurationMicrosecondValue(v.value()), TimeUnit::Nanosecond => ValueData::DurationNanosecondValue(v.value()), }), - Value::Decimal128(v) => { - let (hi, lo) = v.split_value(); - Some(ValueData::Decimal128Value(v1::Decimal128 { hi, lo })) - } + Value::Decimal128(v) => Some(ValueData::Decimal128Value(convert_to_pb_decimal128(v))), Value::List(_) => unreachable!(), }, } diff --git a/src/common/decimal/src/decimal128.rs b/src/common/decimal/src/decimal128.rs index ce23fcf98a97..d742be5876f4 100644 --- a/src/common/decimal/src/decimal128.rs +++ b/src/common/decimal/src/decimal128.rs @@ -110,9 +110,15 @@ impl Decimal128 { } /// Convert from precision, scale, a i128 value which - /// represents by two i64 value(high-64 bit, low-64 bit). + /// represents by i64 + i64 value(high-64 bit, low-64 bit). pub fn from_value_precision_scale(hi: i64, lo: i64, precision: u8, scale: i8) -> Self { - let value = (hi as i128) << 64 | lo as i128; + // 128 64 0 + // +-------+-------+-------+-------+-------+-------+-------+-------+ + // | hi | lo | + // +-------+-------+-------+-------+-------+-------+-------+-------+ + let hi = (hi as u128 & u64::MAX as u128) << 64; + let lo = lo as u128 & u64::MAX as u128; + let value = (hi | lo) as i128; Self::new(value, precision, scale) } } @@ -429,4 +435,30 @@ mod tests { let decimal2 = Decimal128::from_str("1234567890.123").unwrap(); assert_eq!(decimal1.partial_cmp(&decimal2), None); } + + #[test] + fn test_convert_with_i128() { + let test_decimal128_eq = |value| { + let decimal1 = + Decimal128::new(value, DECIMAL128_MAX_PRECISION, DECIMAL128_DEFAULT_SCALE); + let (hi, lo) = decimal1.split_value(); + let decimal2 = Decimal128::from_value_precision_scale( + hi, + lo, + DECIMAL128_MAX_PRECISION, + DECIMAL128_DEFAULT_SCALE, + ); + assert_eq!(decimal1, decimal2); + }; + + test_decimal128_eq(1 << 63); + + test_decimal128_eq(0); + test_decimal128_eq(1234567890); + test_decimal128_eq(-1234567890); + test_decimal128_eq(32781372819372817382183218i128); + test_decimal128_eq(-32781372819372817382183218i128); + test_decimal128_eq(i128::MAX); + test_decimal128_eq(i128::MIN); + } } diff --git a/src/common/time/src/interval.rs b/src/common/time/src/interval.rs index 716602b8e2a8..892e4807c499 100644 --- a/src/common/time/src/interval.rs +++ b/src/common/time/src/interval.rs @@ -258,21 +258,24 @@ impl Interval { } pub fn to_i128(&self) -> i128 { - let mut result = 0; - result |= self.months as i128; - result <<= 32; - result |= self.days as i128; - result <<= 64; - result |= self.nsecs as i128; - result + // 128 96 64 0 + // +-------+-------+-------+-------+-------+-------+-------+-------+ + // | months | days | nanoseconds | + // +-------+-------+-------+-------+-------+-------+-------+-------+ + let months = (self.months as u128 & u32::MAX as u128) << 96; + let days = (self.days as u128 & u32::MAX as u128) << 64; + let nsecs = self.nsecs as u128 & u64::MAX as u128; + (months | days | nsecs) as i128 } pub fn to_i64(&self) -> i64 { - let mut result = 0; - result |= self.days as i64; - result <<= 32; - result |= self.nsecs / NANOS_PER_MILLI; - result + // 64 32 0 + // +-------+-------+-------+-------+-------+-------+-------+-------+ + // | days | milliseconds | + // +-------+-------+-------+-------+-------+-------+-------+-------+ + let days = (self.days as u64 & u32::MAX as u64) << 32; + let milliseconds = (self.nsecs / NANOS_PER_MILLI) as u64 & u32::MAX as u64; + (days | milliseconds) as i64 } pub fn to_i32(&self) -> i32 { @@ -635,9 +638,25 @@ mod tests { #[test] fn test_interval_i128_convert() { - let interval = Interval::from_month_day_nano(1, 1, 1); - let interval_i128 = interval.to_i128(); - assert_eq!(interval_i128, 79228162532711081667253501953); + let test_interval_eq = |month, day, nano| { + let interval = Interval::from_month_day_nano(month, day, nano); + let interval_i128 = interval.to_i128(); + let interval2 = Interval::from_i128(interval_i128); + assert_eq!(interval, interval2); + }; + + test_interval_eq(1, 2, 3); + test_interval_eq(1, -2, 3); + test_interval_eq(1, -2, -3); + test_interval_eq(-1, -2, -3); + test_interval_eq(i32::MAX, i32::MAX, i64::MAX); + test_interval_eq(i32::MIN, i32::MAX, i64::MAX); + test_interval_eq(i32::MAX, i32::MIN, i64::MAX); + test_interval_eq(i32::MAX, i32::MAX, i64::MIN); + test_interval_eq(i32::MIN, i32::MIN, i64::MAX); + test_interval_eq(i32::MAX, i32::MIN, i64::MIN); + test_interval_eq(i32::MIN, i32::MAX, i64::MIN); + test_interval_eq(i32::MIN, i32::MIN, i64::MIN); } #[test] diff --git a/src/operator/src/req_convert/common.rs b/src/operator/src/req_convert/common.rs index 694906b82833..3c63d0f4b075 100644 --- a/src/operator/src/req_convert/common.rs +++ b/src/operator/src/req_convert/common.rs @@ -148,17 +148,17 @@ fn push_column_to_rows(column: Column, rows: &mut [Row]) -> Result<()> { (TimeNanosecond, TimeNanosecondValue, time_nanosecond_values), ( IntervalYearMonth, - IntervalYearMonthValues, + IntervalYearMonthValue, interval_year_month_values ), ( IntervalDayTime, - IntervalDayTimeValues, + IntervalDayTimeValue, interval_day_time_values ), ( IntervalMonthDayNano, - IntervalMonthDayNanoValues, + IntervalMonthDayNanoValue, interval_month_day_nano_values ), (DurationSecond, DurationSecondValue, duration_second_values),