Blizzara commented on code in PR #12112:
URL: https://github.com/apache/datafusion/pull/12112#discussion_r1818961187
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -2299,6 +2306,35 @@ fn from_substrait_literal(
Some(LiteralType::IntervalYearToMonth(IntervalYearToMonth { years,
months })) => {
ScalarValue::new_interval_ym(*years, *months)
}
+ Some(LiteralType::IntervalCompound(IntervalCompound {
+ interval_year_to_month,
+ interval_day_to_second,
+ })) => match (interval_year_to_month, interval_day_to_second) {
+ (
+ Some(IntervalYearToMonth { years, months }),
+ Some(IntervalDayToSecond {
+ days,
+ seconds,
+ subseconds,
+ precision_mode:
+
Some(interval_day_to_second::PrecisionMode::Precision(p)),
+ }),
+ ) => {
+ if *p < 0 || *p > 9 {
+ return plan_err!(
+ "Unsupported Substrait interval day to second
precision: {}",
+ p
+ );
+ }
+ let nanos = *subseconds * i64::pow(10, (*p - 9) as u32);
Review Comment:
That is a very fair question! I think in general the answer is "yes", but
there's a catch - the IntervalDayTime in Arrow only supports millisecond
precision, so it cannot be done with a single i64::pow. It could be done with
two (making it more complicated), or with a fp64::pow (leading to floating
point ops).
But there is an additional question of should we allow the silent
precision-losing operations (and the answer is probably no, at least not w/o
some user config option) so I think that parts needs a better look anyways and
I was thinking of doing it separately.
--
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]
