chore(deps): Bump DataFusion to 54.0.0#2648
Conversation
xanderbailey
force-pushed
the
xb/datafusion_54
branch
3 times, most recently
from
38323e6 to
a59888d
Compare
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Overall LGTM. we can wait until the 0.10 release is done
| } | ||
|
|
||
| fn as_any(&self) -> &dyn Any { | ||
| self |
| } | ||
| Expr::Cast(c) => { | ||
| if c.data_type == DataType::Date32 || c.data_type == DataType::Date64 { | ||
| if *c.field.data_type() == DataType::Date32 || *c.field.data_type() == DataType::Date64 |
There was a problem hiding this comment.
we may need to fix the data type comparison here as well: https://github.com/apache/iceberg-rust/blob/main/crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs#L265
There was a problem hiding this comment.
Thanks @xanderbailey! Took a pass with the 54.0 upgrade guide open. The as_any removal looks complete and correct across all the trait impls and the downcast call sites, and I confirmed the other guide items don't apply here: partition_statistics isn't implemented (so no Arc<Statistics> change), the to_iceberg_predicate match is non-exhaustive so the new Expr variants (Lambda / HigherOrderFunction / LambdaVariable) are safely ignored, and there's no PruningStatistics / datafusion-proto / comparison_coercion usage in the integration.
One outstanding item, which is the same thing @CTTY flagged: DF54 also renamed logical Expr::Cast's data_type field to field: FieldRef. This PR adapts expr_to_predicate.rs:143 but there's a second site at expr_to_predicate.rs:265 in resolve_nan_preserving_reference (added in #2592, after this branch) that still uses cast.data_type. On current main this won't compile until that one also becomes cast.field.data_type(). So I think this just needs a rebase on main + that one-line fix.
xanderbailey
force-pushed
the
xb/datafusion_54
branch
from
5e2205e to
e9f49ef
Compare
|
@xanderbailey do you mind merging in upstream main to see if it resolves that CI failure? Thanks in advance! |
…Fusion 54 DataFusion 54 removed as_any() from several traits including CatalogProviderList. The datafusion integration crate was updated but the playground crate was missed.
xanderbailey
force-pushed
the
xb/datafusion_54
branch
from
e9f49ef to
1d67613
Compare
|
Sorry for the delay, have been on holiday! |
## Which issue does this PR close? - None. CI maintenance. ## What changes are included in this PR? Deletes every `Install protoc` step: five in ci.yml, one in public-api.yml, one in website.yml. The installs were added with the datafusion 48 bump (#1501). That bump pulled in the `substrait` crate, whose build script runs prost-build, and prost-build shells out to protoc. The DataFusion 54 bump (#2648) dropped `substrait` and `prost-build` from the tree, and prost-build was the only thing that ever ran protoc. The remaining `prost`/`prost-derive` dependencies come from datafusion-proto, which ships pregenerated code and has no build script. So nothing in `make build`, the test suite, or the `cargo doc` run in website.yml needs protoc anymore. A side benefit of this simplification is that we remove a dependency on arduino/setup-protoc, which hasn't seen a commit since September 2024 and was printing a Node runtime deprecation warning on every run. ## Are these changes tested? - Verified on a fork based past #2648: all ci.yml and public-api.yml jobs run green without protoc, including the full docker-backed test suite. - `cargo metadata` confirms datafusion-proto and datafusion-proto-common have no build script. `prost-build` is absent from Cargo.lock and nothing in the workspace source references protoc. Co-authored-by: Abanoub Doss <abanoub.doss@gmail.com>
4 participants
Which issue does this PR close?
DataFusion to 54.0.0 was recently released, we should try and track latests releases.
What changes are included in this PR?
ExecutionPlan TableProvider, SchemaProvider, CatalogProvider, CatalogProviderList and PhysicalExpr have all had their
as_anyremoved from the trait. That's really the only change needed here. Upgrade guide documents thisAre these changes tested?