feat: Support Substrait's IntervalCompound type/literal instead of interval-month-day-nano UDT by Blizzara · Pull Request #12112 · apache/datafusion

Blizzara · 2024-08-22T14:10:33Z

Which issue does this PR close?

Rationale for this change

Previously, Substrait didn't have a type & literal for the intervals that combine year/month and day/time, so DF had to use a custom type to support those. substrait-io/substrait#665 however added support for IntervalCompound, which is a superset of DF's IntervalMonthDayNano - so we can switch over.

What changes are included in this PR?

Add support for consuming Substrait's IntervalCompound type/literals (in addition to existing UDTs, for backwards compatibility)
Switch Substrait producing to produce IntervalCompound type/literals

Are these changes tested?

Tested through existing unit tests.

However, as we now don't produce any UDTs, we cannot easily test the UDT-producing code anymore.

Are there any user-facing changes?

Substrait now produces IntervalCompound types instead of interval-month-day-nano UDTs. Both types can be consumed.

github-actions · 2024-10-22T02:00:19Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

Blizzara · 2024-10-28T00:08:40Z

datafusion/substrait/src/variation_const.rs

 /// [`DataType::Interval`]: datafusion::arrow::datatypes::DataType::Interval
 /// [`IntervalUnit::MonthDayNano`]: datafusion::arrow::datatypes::IntervalUnit::MonthDayNano
+#[deprecated(
+    since = "42.1.0",


I guess that should be next version, what will it be? :)

43 I believe: #12470

updated in d527b4e

Blizzara · 2024-10-28T00:09:45Z

datafusion/substrait/src/logical_plan/producer.rs

    }

-    #[test]
-    fn custom_type_literal_extensions() -> Result<()> {


sadly as we no longer produce custom types, there's no easy way to test for them either :/
maybe if we one day implement some UDT support then that can be used for testing here instead. But for now there's no good replacement.

…ano UDT

Blizzara · 2024-10-28T01:43:00Z

this should be good to review, fyi @alamb @vbarua @westonpace @tokoko (feel free to lmk if you'd prefer not to be tagged :))

tokoko · 2024-10-28T09:57:09Z

datafusion/substrait/src/logical_plan/consumer.rs

+                        p
+                    );
+                }
+                let nanos = *subseconds * i64::pow(10, (*p - 9) as u32);


Shouldn't precision handling here and on L2294 for IntervalDayToSecond be similar? Seems like only 0, 3, 6, 9 are supported over there. This one makes more sense to me, just curious why they are different.

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.

sounds good. I think there are two questions (probably unrelated?) to explore here: 1) precision-losing (for p>3) and 2) whether in-between values are valid (p==1, p==2)

alamb · 2024-10-28T11:14:01Z

this should be good to review, fyi @alamb @vbarua @westonpace @tokoko (feel free to lmk if you'd prefer not to be tagged :))

Please do continue to tag me -- thank you very much

alamb

Looks good to me -- thanks @Blizzara

vbarua

2 comments but nothing blocking from me

vbarua · 2024-10-28T18:26:36Z

datafusion/substrait/src/logical_plan/consumer.rs

                            ),
                    }
                } else {
-                    // Kept for backwards compatibility, new plans should include the extension instead


minor: I would suggest updating this warning here and further down to something like

// Kept for backwards compatibility // Producers should use IntervalCompound instead

to make it clearer what/who should be using the new type.

vbarua · 2024-10-28T18:52:27Z

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)),


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).

Yup see #12112 (comment). I think we should change it, maybe with some option so that user can decide if they want to throw or accept the precision loss. But will be a separate PR :)

westonpace

This looks awesome! Only one small question.

westonpace · 2024-10-28T23:24:25Z

datafusion/substrait/src/logical_plan/consumer.rs

+                        p
+                    );
+                }
+                let nanos = *subseconds * i64::pow(10, (*p - 9) as u32);


I'm probably butchering this but should this be 9 - *p instead of *p - 9? If precision is 0 then we want 10^9 right?

No I think you’re right 🙈 math is hard.. I’ll fix this tomorrow. Thanks for catching it!

No I think you’re right 🙈 math is hard.. I’ll fix this tomorrow. Thanks for catching it!

Marking this PR as draft so we don't accidentally merge it until it is fixed

fixed in 79f6341 and added a test in ea8bc4a, thanks again for catching this @westonpace!

Thanks @alamb, I converted back to PR now and I think it's ready to merge :)

alamb · 2024-10-29T13:48:43Z

I'll plan to merge this PR once the CI checks have completed

alamb · 2024-10-29T14:22:35Z

🚀

alamb · 2024-10-29T14:23:04Z

Thanks again @Blizzara @vbarua @westonpace and @tokoko 👥

github-actions bot added the substrait Changes to the substrait crate label Aug 22, 2024

github-actions bot added the Stale PR has not had any activity for some time label Oct 22, 2024

Blizzara force-pushed the avo/substrait-interval-compound branch from d89da8e to 66cb65a Compare October 28, 2024 00:06

Blizzara marked this pull request as ready for review October 28, 2024 00:06

Blizzara commented Oct 28, 2024

View reviewed changes

Blizzara force-pushed the avo/substrait-interval-compound branch from 66cb65a to f1e5176 Compare October 28, 2024 00:44

feat(substrait): use IntervalCompound instead of interval-month-day-n…

e18ec17

…ano UDT

Blizzara force-pushed the avo/substrait-interval-compound branch from f1e5176 to e18ec17 Compare October 28, 2024 01:38

github-actions bot removed the Stale PR has not had any activity for some time label Oct 28, 2024

tokoko reviewed Oct 28, 2024

View reviewed changes

alamb approved these changes Oct 28, 2024

View reviewed changes

tokoko approved these changes Oct 28, 2024

View reviewed changes

Arttu Voutilainen added 3 commits October 28, 2024 08:45

clippy

8b628e0

more clippy

36402bc

even more clippy

94ee836

vbarua approved these changes Oct 28, 2024

View reviewed changes

westonpace approved these changes Oct 28, 2024

View reviewed changes

alamb marked this pull request as draft October 29, 2024 11:34

Arttu Voutilainen added 2 commits October 29, 2024 09:21

fix precision exponent

79f6341

add a test

ea8bc4a

Blizzara marked this pull request as ready for review October 29, 2024 13:38

Arttu Voutilainen added 2 commits October 29, 2024 09:51

update deprecation version

d527b4e

update deprecation comments

8c215ea

alamb merged commit 444a673 into apache:main Oct 29, 2024

Blizzara deleted the avo/substrait-interval-compound branch October 29, 2024 15:15

alamb mentioned this pull request Oct 29, 2024

Oct 28, 2024: This week in DataFusion #13167

Closed

3 tasks

alamb mentioned this pull request Nov 5, 2024

Nov 5. 2024: This week in DataFusion #13265

Closed

3 tasks

Comments

Conversation

Blizzara commented Aug 22, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Oct 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Blizzara commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 28, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

vbarua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 29, 2024

Uh oh!

alamb commented Oct 29, 2024

Uh oh!

alamb commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb Oct 29, 2024 •

edited

Loading