-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make Substrait Schema Structs always non-nullable #15011
Make Substrait Schema Structs always non-nullable #15011
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 reference, reply by @westonpace in Substrait slack: https://substrait.slack.com/archives/C02D7CTQXHD/p1741132969337089?thread_ts=1741128091.732799&cid=C02D7CTQXHD
Issue in Substrait repo: substrait-io/substrait#787
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 didn't see a good place in the existing test suite to add a test for this, would you see any downside to taking the substrait-validator crate on as a dev-dependency and adding a small suite to ensure a set of basic plans are always valid? |
That seems like a (very) good idea to me as producing valid substrait seems quite useful Perhaps we can do it as a follow on PR? |
Thank you @amoeba |
Thanks for chasing this up here and in the core Substrait spec @amoeba 🙇 |
Which issue does this PR close?
Rationale for this change
#12244 reported a larger problem with plan generation which was mostly but not completely fixed by #12245. As noted in #12244, invalid plans could be still be generated.
What changes are included in this PR?
Structs for Schemas are set to always non-nullable.
Are these changes tested?
Yes. I ran the substrait-validator CLI over a plan generated from datafusion built with this patch.
Are there any user-facing changes?
No.