vbarua commented on code in PR #12112:
URL: https://github.com/apache/datafusion/pull/12112#discussion_r1819587798
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1831,8 +1832,13 @@ fn from_substrait_type(
Ok(DataType::Interval(IntervalUnit::YearMonth))
}
r#type::Kind::IntervalDay(_) =>
Ok(DataType::Interval(IntervalUnit::DayTime)),
Review Comment:
Possibly out of scope of this PR, but do we need to checking the precision
on this type now?
Based on
https://github.com/substrait-io/substrait/blob/683f4179a058c2c99c04501b920a48ff372356ff/proto/substrait/type.proto#L132-L140
DataFusion can only handle this type with precision loss when `precision` is
set to 0 or 3 (seconds or milliseconds).
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1842,7 +1848,7 @@ fn from_substrait_type(
),
}
} else {
- // Kept for backwards compatibility, new plans should
include the extension instead
Review Comment:
minor: I would suggest updating this warning here and further down to
something like
```rust
// Kept for backwards compatibility
// Producers should use IntervalCompound instead
```
to make it clearer what/who should be using the new type.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
