substrait-io / substrait

A cross platform way to express data transformation, relational algebra, standardized record expression and plans.
https://substrait.io
Apache License 2.0
1.16k stars 150 forks source link

feat: add precision to IntervalDay and new IntervalCompound type #665

Closed Blizzara closed 1 month ago

Blizzara commented 1 month ago

BREAKING CHANGE: The encoding of IntervalDay literals has changed in a strictly backwards incompatible way. However, the logical meaning across encoding is maintained using a oneof. Moving a field into a oneof makes unset/set to zero unclear with older messages but the fields are defined such that the logical meaning of the two is indistinct. If neither microseconds nor precision is set, the value can be considered a precision 6 value. If you aren't using IntervalDay type, you will not need to make any changes.

BREAKING CHANGE: TypeExpression and Parametertized type protobufs (used to serialize output derivation) are updated to match the now compound nature of IntervalDay. If you use protobuf to serialize output derivation that refer to IntervalDay type, you will need to rework that logic.

fixes #664

jacques-n commented 1 month ago

Per other comment, I think the discussion is complete in #664. Would be great to get PR update.

jacques-n commented 1 month ago

I've updated the PR here. @westonpace @EpsilonPrime @vbarua , can you all take a look?

This is technically a breaking change although the way I did it, it isn't logically one. Nonetheless, I think we should have sufficient +1s before merging.

jacques-n commented 1 month ago

Given @westonpace seeing a lack of clarity about the fact that the literal is backwards compatible, I added an additional comment to the IntervalDay literal to better clarify that consumers need to understand that two different variations are supported. @westonpace and @EpsilonPrime, if you're both good, I'll merge this. Thanks

jacques-n commented 1 month ago

Just need an approval from @vbarua or @EpsilonPrime and/or @cpcloud and then we can merge.

vbarua commented 1 month ago

Taking a look at this today. I want to verify that backwards compatibility of this internally.