Copilot commented on code in PR #56:
URL: https://github.com/apache/fluss-rust/pull/56#discussion_r2553952543
##########
crates/fluss/src/record/arrow.rs:
##########
@@ -543,9 +546,39 @@ pub fn to_arrow_type(fluss_type: &DataType) ->
ArrowDataType {
DataType::String(_) => ArrowDataType::Utf8,
DataType::Decimal(_) => todo!(),
DataType::Date(_) => ArrowDataType::Date32,
- DataType::Time(_) => todo!(),
- DataType::Timestamp(_) => todo!(),
- DataType::TimestampLTz(_) => todo!(),
+ DataType::Time(time_type) => {
+ let precision = time_type.precision();
+ match precision {
+ 0 => ArrowDataType::Time32(TimeUnit::Second),
+ 1..=3 => ArrowDataType::Time32(TimeUnit::Millisecond),
+ 4..=6 => ArrowDataType::Time64(TimeUnit::Microsecond),
+ 7..=9 => ArrowDataType::Time64(TimeUnit::Nanosecond),
+ _ => ArrowDataType::Time32(TimeUnit::Second), // fallback
Review Comment:
The fallback comment is misleading. According to TimeType::MAX_PRECISION
(9), precision values greater than 9 should be considered invalid rather than
silently falling back to Time32(Second). Consider either adding validation to
reject invalid precision values, or updating the comment to clarify this is
handling out-of-range precision values (e.g., '// fallback for invalid
precision > 9').
```suggestion
_ => ArrowDataType::Time32(TimeUnit::Second), // fallback
for invalid precision > 9
```
##########
crates/fluss/src/record/arrow.rs:
##########
@@ -543,9 +546,39 @@ pub fn to_arrow_type(fluss_type: &DataType) ->
ArrowDataType {
DataType::String(_) => ArrowDataType::Utf8,
DataType::Decimal(_) => todo!(),
DataType::Date(_) => ArrowDataType::Date32,
- DataType::Time(_) => todo!(),
- DataType::Timestamp(_) => todo!(),
- DataType::TimestampLTz(_) => todo!(),
+ DataType::Time(time_type) => {
+ let precision = time_type.precision();
+ match precision {
+ 0 => ArrowDataType::Time32(TimeUnit::Second),
+ 1..=3 => ArrowDataType::Time32(TimeUnit::Millisecond),
+ 4..=6 => ArrowDataType::Time64(TimeUnit::Microsecond),
+ 7..=9 => ArrowDataType::Time64(TimeUnit::Nanosecond),
+ _ => ArrowDataType::Time32(TimeUnit::Second), // fallback
+ }
+ }
+ DataType::Timestamp(timestamp_type) => {
+ let precision = timestamp_type.precision();
+ let time_unit = match precision {
+ 0 => TimeUnit::Second,
+ 1..=3 => TimeUnit::Millisecond,
+ 4..=6 => TimeUnit::Microsecond,
+ 7..=9 => TimeUnit::Nanosecond,
+ _ => TimeUnit::Microsecond, // default is 6, so microseconds
Review Comment:
The fallback branches in both Timestamp and TimestampLTz match arms (lines
566 and 577) use Microsecond with the comment 'default is 6, so microseconds'.
This duplicates the logic. Consider extracting a helper function like
`precision_to_time_unit(precision: u32) -> TimeUnit` to avoid code duplication
and ensure consistency between Timestamp and TimestampLTz conversions.
##########
crates/fluss/src/record/arrow.rs:
##########
@@ -543,9 +546,39 @@ pub fn to_arrow_type(fluss_type: &DataType) ->
ArrowDataType {
DataType::String(_) => ArrowDataType::Utf8,
DataType::Decimal(_) => todo!(),
DataType::Date(_) => ArrowDataType::Date32,
- DataType::Time(_) => todo!(),
- DataType::Timestamp(_) => todo!(),
- DataType::TimestampLTz(_) => todo!(),
+ DataType::Time(time_type) => {
+ let precision = time_type.precision();
+ match precision {
+ 0 => ArrowDataType::Time32(TimeUnit::Second),
+ 1..=3 => ArrowDataType::Time32(TimeUnit::Millisecond),
+ 4..=6 => ArrowDataType::Time64(TimeUnit::Microsecond),
+ 7..=9 => ArrowDataType::Time64(TimeUnit::Nanosecond),
+ _ => ArrowDataType::Time32(TimeUnit::Second), // fallback
+ }
+ }
+ DataType::Timestamp(timestamp_type) => {
+ let precision = timestamp_type.precision();
+ let time_unit = match precision {
+ 0 => TimeUnit::Second,
+ 1..=3 => TimeUnit::Millisecond,
+ 4..=6 => TimeUnit::Microsecond,
+ 7..=9 => TimeUnit::Nanosecond,
+ _ => TimeUnit::Microsecond, // default is 6, so microseconds
+ };
+ ArrowDataType::Timestamp(time_unit, None)
+ }
+ DataType::TimestampLTz(timestamp_ltz_type) => {
+ let precision = timestamp_ltz_type.precision();
+ let time_unit = match precision {
+ 0 => TimeUnit::Second,
+ 1..=3 => TimeUnit::Millisecond,
+ 4..=6 => TimeUnit::Microsecond,
+ 7..=9 => TimeUnit::Nanosecond,
+ _ => TimeUnit::Microsecond, // default is 6, so microseconds
+ };
+ // Use UTC as the timezone for local timezone timestamps
Review Comment:
The comment 'Use UTC as the timezone for local timezone timestamps' is
confusing. TimestampLTz represents 'TIMESTAMP WITH LOCAL TIME ZONE', which
typically means the timestamp should be stored in UTC but interpreted in the
local timezone. The implementation is correct, but the comment should clarify
that UTC is used for storage while the local timezone is applied during
interpretation/display.
```suggestion
// Store timestamps in UTC; local timezone is applied during
interpretation/display
```
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -115,13 +115,13 @@ impl JsonSerde for DataType {
}
DataType::Time(_type) => {
- todo!()
+ obj.insert(Self::FIELD_NAME_PRECISION.to_string(),
json!(_type.precision()));
}
DataType::Timestamp(_type) => {
- todo!()
+ obj.insert(Self::FIELD_NAME_PRECISION.to_string(),
json!(_type.precision()));
}
DataType::TimestampLTz(_type) => {
- todo!()
+ obj.insert(Self::FIELD_NAME_PRECISION.to_string(),
json!(_type.precision()));
Review Comment:
The three consecutive match arms (Time, Timestamp, TimestampLTz) have
identical implementations - all insert precision into the JSON object. These
can be combined into a single match arm pattern: `DataType::Time(_type) |
DataType::Timestamp(_type) | DataType::TimestampLTz(_type) => {
obj.insert(Self::FIELD_NAME_PRECISION.to_string(), json!(_type.precision())); }`
--
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]
