chore: cr comment

Co-authored-by: Dennis Zhuang <[email protected]>
Co-authored-by: Yingwen <[email protected]>

src/common/decimal/src/decimal128.rs

@@ -51,15 +51,16 @@ pub struct Decimal128 {
 }
 
 impl Decimal128 {
-    /// Create a new Decimal128 from i128, precision and scale.
-    pub fn new_unchecked(value: i128, precision: u8, scale: i8) -> Self {
+    /// Create a new Decimal128 from i128, precision and scale without any validation.
+    pub fn new(value: i128, precision: u8, scale: i8) -> Self {
         Self {
             value,
             precision,
             scale,
         }
     }
 
+    /// Try new Decimal128 from i128, precision and scale with validation.
     pub fn try_new(value: i128, precision: u8, scale: i8) -> error::Result<Self> {
         // make sure the precision and scale is valid.
         valid_precision_and_scale(precision, scale)?;
@@ -70,6 +71,7 @@ impl Decimal128 {
         })
     }
 
+    /// Return underlying value without precision and scale
     pub fn val(&self) -> i128 {
         self.value
     }
@@ -281,7 +283,7 @@ mod tests {
 
     #[test]
     fn test_common_decimal128() {
-        let decimal = Decimal128::new_unchecked(123456789, 9, 3);
+        let decimal = Decimal128::new(123456789, 9, 3);
         assert_eq!(decimal.to_string(), "123456.789");
 
         let decimal = Decimal128::try_new(123456789, 9, 0);

src/common/grpc/src/select.rs

@@ -71,6 +71,7 @@ macro_rules! convert_arrow_array_to_grpc_vals {
                     return Ok(vals);
                 },
             )+
+            // TODO(QuenKar): support gRPC for Decimal128
             ConcreteDataType::Null(_) | ConcreteDataType::List(_) | ConcreteDataType::Dictionary(_) | ConcreteDataType::Decimal128(_) => unreachable!("Should not send {:?} in gRPC", $data_type),
         }
     }};

src/datatypes/src/data_type.rs

@@ -29,7 +29,7 @@ use serde::{Deserialize, Serialize};
 use crate::error::{self, Error, Result};
 use crate::type_id::LogicalTypeId;
 use crate::types::{
-    BinaryType, BooleanType, DateTimeType, DateType, DecimalType, DictionaryType,
+    BinaryType, BooleanType, DateTimeType, DateType, Decimal128Type, DictionaryType,
     DurationMicrosecondType, DurationMillisecondType, DurationNanosecondType, DurationSecondType,
     DurationType, Float32Type, Float64Type, Int16Type, Int32Type, Int64Type, Int8Type,
     IntervalDayTimeType, IntervalMonthDayNanoType, IntervalType, IntervalYearMonthType, ListType,
@@ -58,8 +58,8 @@ pub enum ConcreteDataType {
     Float32(Float32Type),
     Float64(Float64Type),
 
-    // Decimal type:
-    Decimal128(DecimalType),
+    // Decimal128 type:
+    Decimal128(Decimal128Type),
 
     // String types:
     Binary(BinaryType),
@@ -248,7 +248,7 @@ impl ConcreteDataType {
         }
     }
 
-    pub fn as_decimal(&self) -> Option<DecimalType> {
+    pub fn as_decimal128(&self) -> Option<Decimal128Type> {
         match self {
             ConcreteDataType::Decimal128(d) => Some(*d),
             _ => None,
@@ -479,7 +479,7 @@ impl ConcreteDataType {
     }
 
     pub fn decimal128_datatype(precision: u8, scale: i8) -> ConcreteDataType {
-        ConcreteDataType::Decimal128(DecimalType::new(precision, scale))
+        ConcreteDataType::Decimal128(Decimal128Type::new(precision, scale))
     }
 
     pub fn decimal128_default_datatype() -> ConcreteDataType {

src/datatypes/src/error.rs

@@ -123,8 +123,18 @@ pub enum Error {
     #[snafu(display("Failed to unpack value to given type: {}", reason))]
     TryFromValue { reason: String, location: Location },
 
-    #[snafu(display("Invalid arguments, reason: {}", error))]
-    InvalidArguments {
+    #[snafu(display("Failed to specify the precision {} and scale {}", precision, scale))]
+    InvalidPrecisionOrScale {
+        precision: u8,
+        scale: i8,
+        #[snafu(source)]
+        error: arrow::error::ArrowError,
+        location: Location,
+    },
+
+    #[snafu(display("Value exceeds the precision {} bound", precision))]
+    ValueExceedsPrecision {
+        precision: u8,
         #[snafu(source)]
         error: arrow::error::ArrowError,
         location: Location,

src/datatypes/src/scalars.rs

@@ -420,7 +420,7 @@ mod tests {
 
     #[test]
     fn test_decimal_scalar() {
-        let decimal = Decimal128::new_unchecked(1, 1, 1);
+        let decimal = Decimal128::new(1, 1, 1);
         assert_eq!(decimal, decimal.as_scalar_ref());
         assert_eq!(decimal, decimal.to_owned_scalar());
     }

src/datatypes/src/types.rs

@@ -33,7 +33,7 @@ pub use boolean_type::BooleanType;
 pub use cast::{cast, cast_with_opt};
 pub use date_type::DateType;
 pub use datetime_type::DateTimeType;
-pub use decimal_type::DecimalType;
+pub use decimal_type::Decimal128Type;
 pub use dictionary_type::DictionaryType;
 pub use duration_type::{
     DurationMicrosecondType, DurationMillisecondType, DurationNanosecondType, DurationSecondType,

src/datatypes/src/types/decimal_type.rs

@@ -25,12 +25,12 @@ use crate::vectors::{Decimal128VectorBuilder, MutableVector};
 #[derive(
     Debug, Default, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize,
 )]
-pub struct DecimalType {
+pub struct Decimal128Type {
     precision: u8,
     scale: i8,
 }
 
-impl DecimalType {
+impl Decimal128Type {
     pub fn new(precision: u8, scale: i8) -> Self {
         Self { precision, scale }
     }
@@ -44,7 +44,7 @@ impl DecimalType {
     }
 }
 
-impl DataType for DecimalType {
+impl DataType for Decimal128Type {
     fn name(&self) -> &str {
         "decimal128"
     }

src/datatypes/src/value.rs

@@ -120,7 +120,7 @@ impl Display for Value {
                     .join(", ");
                 write!(f, "{}[{}]", v.datatype.name(), items)
             }
-            Value::Decimal128(v) => write!(f, "{:?}", v),
+            Value::Decimal128(v) => write!(f, "{}", v),
         }
     }
 }
@@ -2255,9 +2255,6 @@ mod tests {
 
     #[test]
     fn test_value_ref_estimated_size() {
-        // MacOS is 48, Ubuntu is 32
-        // assert_eq!(std::mem::size_of::<ValueRef>(), 48);
-
         check_value_ref_size_eq(&ValueRef::Boolean(true), 1);
         check_value_ref_size_eq(&ValueRef::UInt8(1), 1);
         check_value_ref_size_eq(&ValueRef::UInt16(1), 2);
@@ -2334,9 +2331,6 @@ mod tests {
             }),
             85,
         );
-        check_value_ref_size_eq(
-            &ValueRef::Decimal128(Decimal128::new_unchecked(1234, 3, 1)),
-            32,
-        )
+        check_value_ref_size_eq(&ValueRef::Decimal128(Decimal128::new(1234, 3, 1)), 32)
     }
 }