feat(spec): Add primitive data types #18

XuQianJin-Stars · 2024-07-14T06:46:57Z

This PR will add primitive data types.

SteNicholas · 2024-07-16T10:53:20Z

crates/paimon/Cargo.toml

@@ -27,6 +27,7 @@ license.workspace = true
 version.workspace = true

 [dependencies]
+chrono = "0.4.38"


This has been changed in #15.

Xuanwo · 2024-07-17T14:22:43Z

crates/paimon/src/spec/types.rs

+}
+
+pub trait DataTypeTrait {
+    /** Returns whether a value of this type can be {@code null}. */


Hi, please use /// for comments.

Xuanwo · 2024-07-17T14:25:04Z

crates/paimon/src/spec/types.rs

+#[repr(u8)]
+#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
+#[serde(rename_all("camelCase"))]
+pub enum DataTypeRoot {


Are we implementing bitset? How about using bitflags instead?

Xuanwo · 2024-07-17T15:15:42Z

crates/paimon/src/spec/types.rs

+    type_root: DataTypeRoot,
+}
+
+pub trait DataTypeTrait {


Question: We need this trait because we want users to have their own data type?

Xuanwo · 2024-07-17T15:17:11Z

crates/paimon/src/spec/types.rs

+        state.end()
+    }
+
+    fn equals(&self, other: &Self) -> bool {


Rust doesn't implement equals or hash in this way. We need to use Hash and Eq/PartialEq.

Xuanwo · 2024-07-17T15:17:58Z

crates/paimon/src/spec/types.rs

+#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
+pub struct DecimalType {
+    element_type: DataType,
+    precision: u32,


How about using rust_decimal: https://docs.rs/rust_decimal/latest/rust_decimal/

XuQianJin-Stars · 2024-07-18T03:36:05Z

Thanks @Xuanwo @SteNicholas I will modify it according to the comments later.

Aitozi

@XuQianJin-Stars Thanks for your contribution, Left some minor comments.

Aitozi · 2024-07-22T11:56:51Z

crates/paimon/src/spec/types.rs

+    }
+
+    /// Returns true if the data type is with the family.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L214>


The line number seems not correct.

Aitozi · 2024-07-22T12:01:30Z

crates/paimon/src/spec/types.rs

+        }
+    }
+
+    /// Returns true if the data type is with the family.


Incorrect comment

Aitozi · 2024-07-22T12:01:39Z

crates/paimon/src/spec/types.rs

+            .any(|f: &DataTypeFamily| self.is_with_family(f.clone()))
+    }
+
+    /// Returns true if the data type is with the family.


Incorrect comment

Aitozi · 2024-07-22T12:01:56Z

crates/paimon/src/spec/types.rs

+        self.type_root.families().contains(family)
+    }
+
+    /// Returns true if the data type is with the family.


Incorrect comment

Aitozi · 2024-07-22T12:02:20Z

crates/paimon/src/spec/types.rs

+        self.copy(self.is_nullable)
+    }
+
+    /// Returns true if the data type is with the family.


Aitozi · 2024-07-22T12:06:40Z

crates/paimon/src/spec/types.rs

+        self.copy(false)
+    }
+
+    fn as_sql_string(&self) -> String {


This method should be moved to the subclass, Right?

Aitozi · 2024-07-22T12:09:15Z

crates/paimon/src/spec/types.rs

+    }
+
+    pub fn as_sql_string(&self) -> String {
+        format!("ARRAY<{}>", self.element_type.as_sql_string())


The nullability is not displayed in the sql string.

Aitozi · 2024-07-22T12:17:06Z

crates/paimon/src/spec/types.rs

+///
+/// Impl Reference: <https://github.com/apache/paimon/blob/master/paimon-common/src/main/java/org/apache/paimon/types/DataTypeFamily.java>
+#[derive(Debug, Clone, PartialEq, Eq)]
+    pub struct DataTypeFamily: u32 {


I'm curious why we use bitset here instead of the original HashSet ?

It's better to use a bitset here for easier readability and to avoid unnecessary allocation. DataTypeFamily just a u32 which can store in stack but HashSet needs heap allocations.

Get it, thanks.

Aitozi · 2024-07-23T12:32:15Z

crates/paimon/src/spec/types.rs

+    type_root: DataTypeRoot,
+}
+
+impl Display for DataType {


I think we should impl Display for the concrete types such as BigIntType not for the DataType? Otherwise, we can not directly print the subtypes.

Xuanwo

Thanks a lot for your work. We are on the right track. There are some comments for detailed API design, please take a look.

Xuanwo · 2024-07-24T08:17:22Z

crates/paimon/src/spec/types.rs

+/// The root of data type.
+///
+/// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataTypeRoot.java#L49>
+#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, Hash)]


Maybe we can make DataTypeRoot Copy?

Xuanwo · 2024-07-24T08:17:45Z

crates/paimon/src/spec/types.rs

+
+    /// Returns a deep copy of this type. It requires an implementation of {@link #copy(boolean)}.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L120>
+    fn copy_with_nullable(&self) -> Self {


This is Copy in rust.

Xuanwo · 2024-07-24T08:18:11Z

crates/paimon/src/spec/types.rs

+
+    /// Returns a deep copy of this type with possibly different nullability.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L113>
+    fn copy(&self, is_nullable: bool) -> Self {


We can make this API called fn with_nullable(self, is_nullable: bool) -> Self

Xuanwo · 2024-07-24T08:19:03Z

crates/paimon/src/spec/types.rs

+
+    /// Compare two data types without nullable.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L129>
+    fn copy_ignore_nullable(&self) -> Self {


This is duplicated with with_nullable(false)?

Xuanwo · 2024-07-24T08:20:25Z

crates/paimon/src/spec/types.rs

+        self.copy(false)
+    }
+
+    fn serialize_json(&self) -> String {


Ideally, we don't need to provide API like this directly. If we have special requirement for Serialize, we need to implement by hand instead of using #[derive(Serialize)]

Xuanwo · 2024-07-24T08:31:52Z

crates/paimon/src/spec/types.rs

+        }
+    }
+
+    pub fn default_value() -> Self {


Xuanwo · 2024-07-24T08:31:58Z

crates/paimon/src/spec/types.rs

+
+impl Display for FloatType {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        if !self.element_type.is_nullable() {


Xuanwo · 2024-07-24T08:32:46Z

crates/paimon/src/spec/types.rs

+///
+/// Impl Reference: <https://github.com/apache/paimon/blob/master/paimon-common/src/main/java/org/apache/paimon/types/TimestampType.java>.
+#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, Hash)]
+pub struct LocalZonedTimestampType {


Interesting, paimon has a special type for local zone timestamp.

Xuanwo · 2024-07-24T08:33:47Z

crates/paimon/src/spec/types.rs

+                self.precision
+            )
+        } else {
+            write!(f, "TIMESTAMP WITH LOCAL TIME ZONE({})", self.precision)


We can implement like:

write!(f, "TIMESTAMP WITH LOCAL TIME ZONE({})", self.precision)？; if !self.element_type.is_nullable() { write!(f, " NOT NULL")?; } Ok(())

Xuanwo · 2024-07-24T08:33:58Z

crates/paimon/src/spec/types.rs

+        Self::new(true, precision)
+    }
+
+    pub fn default_value() -> Self {


Xuanwo

Mostly LGTM to me now. There all only some small nits.

Xuanwo · 2024-07-25T04:06:23Z

crates/paimon/src/spec/types.rs

+    /// Returns whether the family type of the type equals to the family or not.
+    ///
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L103>
+    fn with_family(&self, family: DataTypeFamily) -> bool {


How about naming it is_family?

Xuanwo · 2024-07-25T04:07:53Z

crates/paimon/src/spec/types.rs

+
+    /// Returns whether the root of the type is part of at least one family of the families or not.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L94>
+    fn is_any_with_family(&self, families: &[DataTypeFamily]) -> bool {


How about naming it is_any_of_family to align with is_any_of?

Xuanwo · 2024-07-25T04:08:54Z

crates/paimon/src/spec/types.rs

+    fn is_any_with_family(&self, families: &[DataTypeFamily]) -> bool {
+        families
+            .iter()
+            .any(|f: &DataTypeFamily| self.with_family(f.clone()))


DataTypeFamily is Copy, so I'm guessing we don't need to clone it.

Have you tried:

families .iter() .any(self.is_family)

Xuanwo · 2024-07-25T04:09:18Z

crates/paimon/src/spec/types.rs

+
+    /// Returns a deep copy of this type with possibly different nullability.
+    /// Impl Reference: <https://github.com/apache/paimon/blob/db8bcd7fdd9c2705435d2ab1d2341c52d1f67ee5/paimon-common/src/main/java/org/apache/paimon/types/DataType.java#L113>
+    fn with_nullable(&self, is_nullable: bool) -> Self {


We can place construct API in the front to make it easier to find.

Xuanwo

Great, thanks a lot for your effort!

Xuanwo · 2024-07-25T06:29:21Z

cc @SteNicholas, @Aitozi and @JingsongLi for another look.

Aitozi · 2024-07-25T06:42:33Z

I have one more question, Why the DataType is not defined as trait ? If not, how the DataType used in other class. eg: org.apache.paimon.types.DataField#asSQLString the type string can not be obtained ? CC @Xuanwo

Xuanwo · 2024-07-25T07:34:40Z

I have one more question, Why the DataType is not defined as trait ? If not, how the DataType used in other class. eg: org.apache.paimon.types.DataField#asSQLString the type string can not be obtained ? CC @Xuanwo

Hi, thanks for bringing this up. I don't quite understand the issue you mentioned.

Are you trying to say that it's possible for users to implement they own data types and we don't know how to display those types? If so, I believe current implemantion of Display is wrong:

impl Display for ArrayType {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        if self.element_type.is_nullable() {
            write!(f, "ARRAY")
        } else {
            write!(f, "ARRAY NOT NULL")
        }
    }
}

This is the part of json represent of array. In fact, we need to use write!(f, "ARRAY<{}>, element_type)" here.

Xuanwo · 2024-07-25T07:47:11Z

After some offline discussion with @Aitozi, I got his point now. I think we need to build an enum for DataType instead of just DataTypeRoot in which we carries the entire data type information. We can implement this as follow-up of this PR.

Aitozi · 2024-07-25T08:11:24Z

An enum DataType make sense to me. +1 for the current shape. Let's move for next.

Aitozi

+1

SteNicholas reviewed Jul 16, 2024

View reviewed changes

Xuanwo reviewed Jul 17, 2024

View reviewed changes

XuQianJin-Stars force-pushed the types branch from 6033e19 to f2490de Compare July 20, 2024 07:50

Aitozi reviewed Jul 22, 2024

View reviewed changes

Aitozi reviewed Jul 23, 2024

View reviewed changes

XuQianJin-Stars added 11 commits July 24, 2024 10:04

feat(spec): Add primitive data types

68f496d

feat(spec): Add primitive data types

2094216

feat(spec): Add primitive data types

06068d7

feat(spec): Add primitive data types

5c5db0b

feat(spec): Add primitive data types

79d1564

feat(spec): Add primitive data types

3e1fed9

feat(spec): Add primitive data types

bb78dfa

feat(spec): Add primitive data types

18a158a

feat(spec): Add primitive data types

edc15f6

feat(spec): Add primitive data types

6943c7b

feat(spec): Add primitive data types

e43f41b

XuQianJin-Stars force-pushed the types branch from e652b57 to e43f41b Compare July 24, 2024 02:04

feat(spec): Add primitive data types

3b66d34

Xuanwo reviewed Jul 24, 2024

View reviewed changes

XuQianJin-Stars added 5 commits July 25, 2024 10:17

feat(spec): Add primitive data types

f6cb291

feat(spec): Add primitive data types

48e3b30

feat(spec): Add primitive data types

25479de

feat(spec): Add primitive data types

b302655

feat(spec): Add primitive data types

167a048

Xuanwo approved these changes Jul 25, 2024

View reviewed changes

feat(spec): Add primitive data types

f4f8474

Xuanwo approved these changes Jul 25, 2024

View reviewed changes

Aitozi approved these changes Jul 25, 2024

View reviewed changes

Aitozi merged commit ce42c78 into apache:main Jul 25, 2024
7 checks passed

XuQianJin-Stars deleted the types branch September 6, 2024 10:46

feat(spec): Add primitive data types #18

feat(spec): Add primitive data types #18

Conversation

XuQianJin-Stars commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuQianJin-Stars commented Jul 18, 2024

Aitozi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aitozi Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Xuanwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo left a comment

Choose a reason for hiding this comment

Xuanwo commented Jul 25, 2024

Aitozi commented Jul 25, 2024 • edited Loading

Xuanwo commented Jul 25, 2024 • edited Loading

Xuanwo commented Jul 25, 2024

Aitozi commented Jul 25, 2024

Aitozi left a comment

Choose a reason for hiding this comment

Aitozi Jul 23, 2024 •

edited

Loading

Aitozi commented Jul 25, 2024 •

edited

Loading

Xuanwo commented Jul 25, 2024 •

edited

Loading