Copilot commented on code in PR #310:
URL: https://github.com/apache/fluss-rust/pull/310#discussion_r2802950152
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -594,6 +598,10 @@ impl TimeType {
self.precision
}
+ pub fn is_nullable(&self) -> bool {
+ self.nullable
+ }
+
Review Comment:
This method is inconsistent with the existing codebase pattern. None of the
other type implementations (BooleanType, IntType, CharType, BinaryType,
ArrayType, MapType, etc.) provide an is_nullable method. The nullable status
should be accessed through the parent DataType enum's is_nullable method
instead. Consider removing this method and updating the tests to either wrap
the TimeType in a DataType, or test nullability through other means (e.g.,
checking the string representation with to_string).
```suggestion
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -717,6 +729,10 @@ impl TimestampLTzType {
self.precision
}
+ pub fn is_nullable(&self) -> bool {
+ self.nullable
+ }
+
Review Comment:
This method is inconsistent with the existing codebase pattern. None of the
other type implementations (BooleanType, IntType, CharType, BinaryType,
ArrayType, MapType, etc.) provide an is_nullable method. The nullable status
should be accessed through the parent DataType enum's is_nullable method
instead. Consider removing this method and updating the tests to either wrap
the TimestampLTzType in a DataType, or test nullability through other means
(e.g., checking the string representation with to_string).
```suggestion
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1431,6 +1452,65 @@ fn test_decimal_invalid_scale() {
);
}
+// ============================================================================
+// DecimalType validation tests - edge cases
+// ============================================================================
+
+#[test]
+fn test_decimal_valid_precision_and_scale() {
+ // Valid: precision=10, scale=2
+ let result = DecimalType::with_nullable(true, 10, 2);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 10);
+ assert_eq!(decimal.scale(), 2);
+ assert!(decimal.is_nullable());
+
+ // Valid: precision=38, scale=0
+ let result = DecimalType::with_nullable(true, 38, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 38);
+ assert_eq!(decimal.scale(), 0);
+
+ // Valid: precision=1, scale=0
+ let result = DecimalType::with_nullable(false, 1, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 1);
+ assert_eq!(decimal.scale(), 0);
+ assert!(!decimal.is_nullable());
+}
+
+#[test]
Review Comment:
Consider adding a test case for the edge case where scale equals precision
(e.g., DecimalType::with_nullable(true, 10, 10)). While this is a valid case
according to the validation logic (line 475 checks scale > precision), it's an
important boundary condition that should be explicitly tested.
```suggestion
#[test]
fn test_decimal_scale_equals_precision_boundary() {
// Boundary: precision=10, scale=10 (scale == precision is valid)
let result = DecimalType::with_nullable(true, 10, 10);
assert!(result.is_ok());
let decimal = result.unwrap();
assert_eq!(decimal.precision(), 10);
assert_eq!(decimal.scale(), 10);
assert!(decimal.is_nullable());
}
#[test]
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1469,3 +1568,79 @@ fn test_timestamp_ltz_invalid_precision() {
.contains("Timestamp with local time zone precision must be
between 0 and 9")
);
}
+
+// ============================================================================
+// RowType projection tests
+// ============================================================================
+
+#[test]
+fn test_row_type_project_valid_indices() {
+ // Create a 3-column row type
+ let row_type = RowType::with_data_types_and_field_names(
+ vec![DataTypes::int(), DataTypes::string(), DataTypes::bigint()],
+ vec!["id", "name", "age"],
+ );
+
+ // Valid projection by indices: [0, 2]
+ let projected = row_type.project(&[0, 2]).unwrap();
+ assert_eq!(projected.fields().len(), 2);
+ assert_eq!(projected.fields()[0].name, "id");
+ assert_eq!(projected.fields()[1].name, "age");
+}
+
+#[test]
Review Comment:
Consider adding a test case for projecting with an empty array (e.g.,
row_type.project(&[])). This edge case should result in an empty RowType, but
it's worth explicitly testing to ensure it's handled correctly.
```suggestion
#[test]
fn test_row_type_project_empty_indices() {
// Create a 3-column row type
let row_type = RowType::with_data_types_and_field_names(
vec![DataTypes::int(), DataTypes::string(), DataTypes::bigint()],
vec!["id", "name", "age"],
);
// Projection with an empty indices array should yield an empty RowType
let projected = row_type.project(&[]).unwrap();
assert_eq!(projected.fields().len(), 0);
}
#[test]
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1431,6 +1452,65 @@ fn test_decimal_invalid_scale() {
);
}
+// ============================================================================
+// DecimalType validation tests - edge cases
+// ============================================================================
+
+#[test]
+fn test_decimal_valid_precision_and_scale() {
+ // Valid: precision=10, scale=2
+ let result = DecimalType::with_nullable(true, 10, 2);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 10);
+ assert_eq!(decimal.scale(), 2);
+ assert!(decimal.is_nullable());
+
+ // Valid: precision=38, scale=0
+ let result = DecimalType::with_nullable(true, 38, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 38);
+ assert_eq!(decimal.scale(), 0);
+
+ // Valid: precision=1, scale=0
+ let result = DecimalType::with_nullable(false, 1, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 1);
+ assert_eq!(decimal.scale(), 0);
+ assert!(!decimal.is_nullable());
+}
+
+#[test]
+fn test_decimal_invalid_precision_zero() {
+ // Invalid: precision=0 (edge case not covered by existing tests)
+ let result = DecimalType::with_nullable(true, 0, 0);
+ assert!(result.is_err());
+ assert!(
+ result
+ .unwrap_err()
+ .to_string()
+ .contains("Decimal precision must be between 1 and 38")
+ );
+}
+
+
+// ============================================================================
+// TimeType validation tests
Review Comment:
The comment has trailing whitespace. Remove the trailing space at the end of
the comment line.
```suggestion
// TimeType validation tests
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -655,6 +663,10 @@ impl TimestampType {
self.precision
}
+ pub fn is_nullable(&self) -> bool {
+ self.nullable
+ }
+
Review Comment:
This method is inconsistent with the existing codebase pattern. None of the
other type implementations (BooleanType, IntType, CharType, BinaryType,
ArrayType, MapType, etc.) provide an is_nullable method. The nullable status
should be accessed through the parent DataType enum's is_nullable method
instead. Consider removing this method and updating the tests to either wrap
the TimestampType in a DataType, or test nullability through other means (e.g.,
checking the string representation with to_string).
```suggestion
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1444,6 +1524,25 @@ fn test_time_invalid_precision() {
);
}
+// ============================================================================
+// TimestampType validation tests
Review Comment:
The comment has trailing whitespace. Remove the trailing space at the end of
the comment line.
```suggestion
// TimestampType validation tests
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1469,3 +1568,79 @@ fn test_timestamp_ltz_invalid_precision() {
.contains("Timestamp with local time zone precision must be
between 0 and 9")
);
}
+
+// ============================================================================
+// RowType projection tests
+// ============================================================================
+
+#[test]
+fn test_row_type_project_valid_indices() {
+ // Create a 3-column row type
+ let row_type = RowType::with_data_types_and_field_names(
+ vec![DataTypes::int(), DataTypes::string(), DataTypes::bigint()],
+ vec!["id", "name", "age"],
+ );
+
+ // Valid projection by indices: [0, 2]
+ let projected = row_type.project(&[0, 2]).unwrap();
+ assert_eq!(projected.fields().len(), 2);
+ assert_eq!(projected.fields()[0].name, "id");
+ assert_eq!(projected.fields()[1].name, "age");
+}
+
+#[test]
Review Comment:
Consider adding a test case for projecting with duplicate indices (e.g.,
row_type.project(&[0, 0, 1])). Based on the implementation, this appears to
allow duplicates, but it's worth explicitly testing this edge case to document
the expected behavior.
```suggestion
#[test]
fn test_row_type_project_duplicate_indices() {
// Create a 3-column row type
let row_type = RowType::with_data_types_and_field_names(
vec![DataTypes::int(), DataTypes::string(), DataTypes::bigint()],
vec!["id", "name", "age"],
);
// Projection with duplicate indices: [0, 0, 1]
let projected = row_type.project(&[0, 0, 1]).unwrap();
assert_eq!(projected.fields().len(), 3);
assert_eq!(projected.fields()[0].name, "id");
assert_eq!(projected.fields()[1].name, "id");
assert_eq!(projected.fields()[2].name, "name");
}
#[test]
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -1431,6 +1452,65 @@ fn test_decimal_invalid_scale() {
);
}
+// ============================================================================
+// DecimalType validation tests - edge cases
+// ============================================================================
+
+#[test]
+fn test_decimal_valid_precision_and_scale() {
+ // Valid: precision=10, scale=2
+ let result = DecimalType::with_nullable(true, 10, 2);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 10);
+ assert_eq!(decimal.scale(), 2);
+ assert!(decimal.is_nullable());
+
+ // Valid: precision=38, scale=0
+ let result = DecimalType::with_nullable(true, 38, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 38);
+ assert_eq!(decimal.scale(), 0);
+
+ // Valid: precision=1, scale=0
+ let result = DecimalType::with_nullable(false, 1, 0);
+ assert!(result.is_ok());
+ let decimal = result.unwrap();
+ assert_eq!(decimal.precision(), 1);
+ assert_eq!(decimal.scale(), 0);
+ assert!(!decimal.is_nullable());
+}
+
+#[test]
+fn test_decimal_invalid_precision_zero() {
+ // Invalid: precision=0 (edge case not covered by existing tests)
+ let result = DecimalType::with_nullable(true, 0, 0);
+ assert!(result.is_err());
+ assert!(
+ result
+ .unwrap_err()
+ .to_string()
+ .contains("Decimal precision must be between 1 and 38")
+ );
+}
+
+
Review Comment:
There's an extra blank line here. Remove one of the consecutive blank lines
to maintain consistent spacing throughout the file.
```suggestion
```
##########
crates/fluss/src/metadata/datatype.rs:
##########
@@ -497,6 +497,10 @@ impl DecimalType {
self.scale
}
+ pub fn is_nullable(&self) -> bool {
+ self.nullable
+ }
+
Review Comment:
This method is inconsistent with the existing codebase pattern. None of the
other type implementations (BooleanType, IntType, CharType, BinaryType,
ArrayType, MapType, etc.) provide an is_nullable method. The nullable status
should be accessed through the parent DataType enum's is_nullable method
instead. Consider removing this method and updating the tests to either wrap
the DecimalType in a DataType, or test nullability through other means (e.g.,
checking the string representation with to_string).
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
