-
Notifications
You must be signed in to change notification settings - Fork 1.7k
chore: bump substrait version to 0.60.0 to use substrait spec v0.75.0
#17866
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
Conversation
|
Looks like substrait 0.60 bumps their msrv to 1.88: https://github.com/substrait-io/substrait-rs/releases/tag/v0.60.0 We'll need to wait until Rust 1.91 releases end of October so we can bump our msrv to 1.88 per our policy: datafusion/docs/source/user-guide/introduction.md Lines 198 to 210 in 297e537
|
|
Folks do you think #17933 failures are related to this PR? |
I have a work in progress to upgrade here: I confirm the prost version was upgraded |
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.
|
|
||
| impl From<Extensions> for Vec<SimpleExtensionDeclaration> { | ||
| // Silence deprecation warnings for `extension_uri_reference` during the uri -> urn migration | ||
| // See: https://github.com/substrait-io/substrait/issues/856 |
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.
Do you plan a follow on PR to update DataFusion to use a non-deprecated API?
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.
Yes I am leading the charge on the substrait side of things to do this migration from uris -> urns. You can find more info here.
It may take some time because it will require getting the rest of the ecosystem off of uris first.
(However, I am aware that datafusion doesn't actually use the extension uri / urn information)
0.60.0 to use substrait spec v0.75.0
|
FYI @gabotechs |
|
Code LGTM, however the aggressive MSVR in TBH the MSVR bump in Options that I see is:
As 1.91 will just be released in a couple of days, I'd advocate for waiting until DataFusion's MSVR is set to 1.88 |
Maybe we can ask @mbrobbel if we could move the MSRV back in a subsequent version of substrait rs 🤔 |
Yes, we can revert to 1.85. It was bumped to get https://blog.rust-lang.org/2025/06/26/Rust-1.88.0/#let-chains. |
https://github.com/substrait-io/substrait-rs/releases/tag/v0.62.0 |
|
@gabotechs I just pushed a change to incorporate @mbrobbel's version bump, so the MSRV shouldn't be an issue anymore. |
| name = "aws-lc-rs" | ||
| version = "1.14.0" | ||
| version = "1.14.1" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "94b8ff6c09cd57b16da53641caa860168b88c172a5ee163b0288d3d6eea12786" | ||
| checksum = "879b6c89592deb404ba4dc0ae6b58ffd1795c78991cbb5b8bc441c48a070440d" | ||
| dependencies = [ |
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.
🤔 there seems to be several version upgrades for unrelated packages in the Cargo.lock file. The aws-lc-rs and other aws related packages should have nothing to do with the substrait version, so I'd expect them to remain as they were.
I would revert the changes in this Cargo.lock and run cargo build again just so that Cargo updates just whatever is strictly necessary.
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.
🚢 let's go!
|
@gabotechs perhaps you would like to merge this PR (when the CI passes) to make sure your github credentials are connected correctly to permit write access? |
I'm still pending on that, it seems like it might still take some days. If by the time you wake you still don't see this merged, feel free to hit that merge button. |
…18310) ## Which issue does this PR close? ## Rationale for this change The `extended` tests rely on the checkout of datafusion-testing (that has the expected results for the sqlite sqllogictest suite) However, we don't currently run the extended tests when that pin is changed so we could potentially break CI on main if we don't catch changes in code review (this just happened to me in #17866 (review)) ## What changes are included in this PR? 1. Run extended CI tests on changes to datafusion-testing ## Are these changes tested? I tested this in PR - #18311 - #18312 ## Are there any user-facing changes? No

Which issue does this PR close?
What changes are included in this PR?
Bump the
substraitversion tov0.75.0by bumpingsubstrait-rstov0.60.0.This PR was originally dependent on this PR to update the versions of some common dependencies, but that PR is now merged in.
Are these changes tested?
There are no tests here, but there is no change to any logic within datafusion. It is simply a bump in a dependency. Technically the public API does change, but as noted in the issue description, there is no change to internal logic because uri / urn from substrait plans are not used.
Are there any user-facing changes?
Yes. Previously substrait plans of spec version
v0.74.0were accepted, and nowv0.75.0is accepted. However, this is a backwards compatible change. The only difference is the inclusion of additional urn-based fields in substrait plans. In a later PR, the old uri-based fields will be dropped, which will be a breaking change.