Blizzara commented on PR #11471: URL: https://github.com/apache/datafusion/pull/11471#issuecomment-2298560063
> Substrait has restricted the value range of its two interval types to [-10,000..10,000] years ([ref](https://substrait.io/types/type_classes/#simple-types)) Yeah.. however no-one cares about those limits currently afaik, and I don't know if it's good enough reason to reduce interoperability for values that fall within that (reasonably large :D) range. As the limit is just a written text, DF -> DF roundtrip would anyways work outside of those limits too. > * Our interval types have different ranges with substraits. E.g., `IntervalDayTime` may overflow [here](https://github.com/apache/datafusion/pull/11471/files#diff-474e53672159d74dae38992a914a74eba81b8350ebe161f11d755f06414ed7b4R1779) because it only has one `i32` as millisecond for substrait's two `i32`s (second and millisecond). Good catch - however, if I think this right, that means we may fail to consume some Substrait plans - but that's true anyways, even if we were to use Substrait UDTs for intervals, right? Now we fail to consume the too-large values, while with UDTs we'd fail to consume everything. Since it's DF that has a more restricted type, any DF-created plan is guaranteed to be within the limits of not overflowing. As such, I think it makes sense to both produce and consume the Substrait DayTime interval type, given it's able to represent all DF DayTime intervals. Did I miss something? 😅 -- 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]
