-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update substrait to 0.57.1
#274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on getting this moving into the right direction. This is a great first step towards bringing the validator up to where it should be.
I suspect we will want to avoid releases until we address all of the basic requirements of the latest version (such as enforcing that function names have their accepted argument types in their names) but that shouldn't hold up this first, great step.
@@ -38,8 +38,13 @@ enum LiteralValue { | |||
/// May be used only for binary and FixedBinary. | |||
Binary(Vec<u8>), | |||
|
|||
/// May be used only for IntervalYearToMonth and IntervalDayToSecond. | |||
Interval(i64, i64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reject use of Interval as deprecated at this point or just return an error as we would likely do once the old fields are removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still new to this code base, but I think this is an internal class that is not exported; it does not have a 1:1 match with the public protobuf files. So there isn't a way to do Interval
(and not IntervalYearToMonth
and IntervalDayToSecond
) in protobufs right now - there's nothing to deprecate or return an error on.
Does that make sense? (Or am I missing something?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Thanks for the comments! I'm working through them now 👍 |
Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>
Required for test case tpc-h01 to pass, where `days: 1` is specified, but subseconds and precision are not.
OK - I've addressed all the comments from code review. There's a couple above that I'm not sure I fully handled - let me know what you think! And thank you both for your quick reviews, @EpsilonPrime and @mbrobbel ! |
Thanks for the reviews! The last thing seems to be the commit lints. I can do some squashing and rebasing to address, although I'm generally hesitant to change history on publicly available commits; it will make the review comments above hard to track. Is that worth doing here? |
No, it doesn't matter. This repo uses squash merge, so it'll use the PR title as merge commit message. |
The validator seemed to be a bit out of date with the main substrait repo, so I decided to see what it would take to update it to current substrait. And... it wasn't too bad.
For new features, I simply added them to the validator as
NotYetImplemented
, so they should emit warnings (not errors).The only place I added actual support was for
IntervalDayToSeconds
, which should now accept theprecision
/subseconds
variety, and not just the deprecatedmicroseconds
.