-
Notifications
You must be signed in to change notification settings - Fork 18
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: upgrade to datafusion 42 and arrow 53 #59
Conversation
Context on removing Expr::Sort apache/datafusion#12177 |
c4bcc4e
to
3dc7e1b
Compare
@@ -1,2405 +0,0 @@ | |||
# This file is automatically @generated by Cargo. |
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.
Note this lock file was unused as it wasn't the toplevel workspace file. Should I commit the toplevel lock file?
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.
Hey @nathanielc, yes we should probably delete it and use the top-level Cargo.lock file.
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.
Looks like the toplevel Cargo.lock file has been explicitly ignored in the repo, as this repo only produces library crates and no binary crates that seems like a good decision. I'll leave the lock file out.
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 @nathanielc
Cargo.toml
Outdated
async-stream = "0.3.5" | ||
async-trait = "0.1.81" | ||
datafusion = "42.0.0" | ||
# XXX use the release version on crates.io |
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.
Could we use the crate version from crates.io here instead of the path?
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 think we should do both, otherwise the code will not be able to build locally as the server and table provider crates depend on the federation crate. I had to look it up to refresh my memory, see https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#local-paths-in-published-crates. In short when using both path and version, cargo uses the path when building locally and the version when publishing to the registry.
Both datafusion and arrow had recent releases. This change updates the code to used those new versions. The biggest changes were related to rewriting table scans in expressions as Expr::Sort was removed and options were added to Expr::Wildcard. Finally, I did a little cleanup of workspace dependencies as a few were duplicated across the various members.
b9b285b
to
fe24a08
Compare
@hozan23 Thanks for the quick review. Is there a process for cutting new releases of this repo? Seems like it would be good to publish a 0.3.0, since its a breaking change to use a new version of datafusion. |
Thanks @nathanielc, just published 0.3.0 version. |
Both datafusion and arrow had recent releases. This change updates the code to used those new versions.
The biggest changes were related to rewriting table scans in expressions as Expr::Sort was removed and options were added to Expr::Wildcard.
Finally, I did a little cleanup of workspace dependencies as a few were duplicated across the various members.