chore: upgrade DataFusion to 54#7793
Open
wjones127 wants to merge 4 commits into
Open
Conversation
geodatafusion 0.5.0 supports DataFusion 54, which was the last remaining blocker. This bumps DataFusion 53 -> 54, and transitively geodatafusion 0.4 -> 0.5, sqlparser 0.61 -> 0.62, and substrait 0.62 -> 0.63. Arrow stays at 58. Migration for DataFusion 54 breaking changes: - `as_any` was removed from the `ExecutionPlan`, `PhysicalExpr`, `ScalarUDFImpl`, `TableProvider`, `SchemaProvider`, `CatalogProvider`, and `CatalogProviderList` traits. Drop the impls and downcast via the inherent `dyn Trait::downcast_ref`/`is` instead of `.as_any().downcast_ref()`. - `ExecutionPlan::partition_statistics` now returns `Arc<Statistics>`. - `Expr::Cast`/`TryCast` hold a `FieldRef` instead of a `DataType`. - `AnalyzeExec::new` gained a `metric_categories` argument and `MetricType::SUMMARY` was renamed to `MetricType::Summary`. - sqlparser 0.62 wraps the LIKE escape character in `ValueWithSpan`. - substrait 0.63 adds `RexType::Lambda`/`LambdaInvocation` and replaces extension URIs with URNs. - Migrate the deprecated `SimplifyContext::default()` builder to `SimplifyContext::builder().build()`. `create_aggregate_expr_and_maybe_filter` is kept under `#[allow(deprecated)]`; migrating to `LoweredAggregateBuilder` is left as follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR upgrades DataFusion integrations from 53 to 54, updates planner and Substrait APIs, migrates execution-plan statistics and downcasting interfaces, and adjusts scalar expression handling, tests, and language bindings. ChangesDataFusion 54 compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
A-python
The substrait 0.63 bump removed the extension-URI API (`SimpleExtensionUri`, `Plan.extension_uris`, `ExtensionFunction.extension_uri_reference`) in favor of URNs. This test file was missed in the DF54 migration because it only compiles under the `substrait` feature, which the default workspace clippy/build does not enable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rder-independent DataFusion 54's optimizer reorders the pushed-down HashJoinExec projection in the full-schema delete-by-source case, moving `__merge_source_sentinel` ahead of `_rowid` to match the consuming ProjectionExec's column order. The plan is still correct (indices line up), but the positional `projection=[_rowid` assertion no longer held. Assert instead that the join keeps a pushed-down projection containing `_rowid`, which is what the test actually cares about. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pylance now exports a datafusion-ffi 54 TableProvider, but the PyPI `datafusion` package was pinned to 53. The mismatched FFI struct layout segfaulted `test_table_provider.py::test_table_loading` when datafusion 53 consumed the v54 provider via `register_table`. Bump the dependency (and the test-group pin) to 54 so both sides share the same FFI ABI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-datafusion/src/planner.rs`:
- Around line 797-800: Update the escape handling shared by SQLExpr::ILike and
SQLExpr::Like so a SingleQuotedString is accepted only when it contains exactly
one character; reject empty or multi-character strings with an input error
instead of using char.chars().next() and silently altering the predicate.
In `@rust/lance-datafusion/src/substrait.rs`:
- Around line 167-169: Ensure Lambda and LambdaInvocation expressions are
rejected on every expression-parse path, not only when schema references are
remapped. Update the parsing flow around remap_expr_references() to traverse and
validate expressions unconditionally while preserving remapping for pruned
schemas, and add a regression test using an unpruned schema that verifies the
invalid-input error.
In `@rust/lance/src/io/exec/count_from_mask.rs`:
- Around line 512-516: Move the create_aggregate_expr_and_maybe_filter import
out of count_star_expr and into the mod tests import block, keeping the
deprecation allowance applied to both the import and its use in the function.
In `@rust/lance/src/io/exec/filtered_read.rs`:
- Around line 2057-2060: Replace the unconditional Statistics clones at both
mutation sites with Arc::unwrap_or_clone so unique Arc allocations are reused:
update the partition_statistics handling in
rust/lance/src/io/exec/filtered_read.rs (lines 2057-2060) and the corresponding
.as_ref().clone() in rust/lance/src/io/exec/rowids.rs (line 291).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9dceccfe-8778-492d-93b1-90a1d2522a23
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lockpython/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
Cargo.tomljava/lance-jni/Cargo.tomlpython/Cargo.tomlpython/pyproject.tomlrust/lance-datafusion/src/exec.rsrust/lance-datafusion/src/planner.rsrust/lance-datafusion/src/substrait.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-namespace-datafusion/src/catalog.rsrust/lance-namespace-datafusion/src/schema.rsrust/lance/src/datafusion/dataframe.rsrust/lance/src/datafusion/logical_plan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/brute_force_vector.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/btree.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/dedup_scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/fts.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/scan.rsrust/lance/src/dataset/mem_wal/memtable/scanner/exec/vector.rsrust/lance/src/dataset/mem_wal/scanner/exec/bloom_guard.rsrust/lance/src/dataset/mem_wal/scanner/exec/coalesce_first.rsrust/lance/src/dataset/mem_wal/scanner/exec/generation_tag.rsrust/lance/src/dataset/mem_wal/scanner/exec/pk_block_filter.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/schema_evolution/optimize.rsrust/lance/src/dataset/tests/dataset_aggregate.rsrust/lance/src/dataset/udtf.rsrust/lance/src/dataset/write/merge_insert.rsrust/lance/src/dataset/write/merge_insert/exec/delete.rsrust/lance/src/dataset/write/merge_insert/exec/write.rsrust/lance/src/io/exec/count_from_mask.rsrust/lance/src/io/exec/count_pushdown.rsrust/lance/src/io/exec/filter.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/fts.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/optimizer.rsrust/lance/src/io/exec/pushdown_scan.rsrust/lance/src/io/exec/rowids.rsrust/lance/src/io/exec/scalar_index.rsrust/lance/src/io/exec/scan.rsrust/lance/src/io/exec/take.rsrust/lance/src/io/exec/testing.rsrust/lance/src/io/exec/utils.rsrust/lance/tests/count_pushdown/mod.rs
💤 Files with no reviewable changes (12)
- rust/lance/src/dataset/udtf.rs
- rust/lance/src/dataset/mem_wal/scanner/exec/coalesce_first.rs
- rust/lance/src/io/exec/testing.rs
- rust/lance-index/src/scalar/rtree/sort/hilbert_sort.rs
- rust/lance/src/dataset/write/merge_insert/exec/delete.rs
- rust/lance/src/dataset/write/merge_insert/exec/write.rs
- rust/lance-namespace-datafusion/src/catalog.rs
- rust/lance/src/dataset/mem_wal/scanner/exec/pk_block_filter.rs
- rust/lance/src/dataset/mem_wal/scanner/exec/bloom_guard.rs
- rust/lance/src/dataset/mem_wal/scanner/exec/generation_tag.rs
- rust/lance-namespace-datafusion/src/schema.rs
- rust/lance/src/io/exec/utils.rs
Comment on lines
+797
to
+800
| Some(ValueWithSpan { | ||
| value: Value::SingleQuotedString(char), | ||
| .. | ||
| }) => char.chars().next(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant regions with line numbers.
wc -l rust/lance-datafusion/src/planner.rs
sed -n '760,840p' rust/lance-datafusion/src/planner.rs
sed -n '800,840p' rust/lance-datafusion/src/planner.rs
sed -n '1200,1245p' rust/lance-datafusion/src/planner.rs
# Search for LIKE/escape related helpers and tests.
rg -n "ESCAPE|escape|Like" rust/lance-datafusion/src/planner.rs rust/lance-datafusion -g '!**/target/**'Repository: lance-format/lance
Length of output: 10362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l rust/lance-datafusion/src/planner.rs
sed -n '760,840p' rust/lance-datafusion/src/planner.rs
sed -n '1200,1245p' rust/lance-datafusion/src/planner.rs
rg -n "ESCAPE|escape|Like" rust/lance-datafusion/src/planner.rs rust/lance-datafusion -g '!**/target/**'Repository: lance-format/lance
Length of output: 8744
🏁 Script executed:
echo testRepository: lance-format/lance
Length of output: 161
Reject invalid LIKE escape strings. Both SQLExpr::ILike and SQLExpr::Like should require exactly one escape character; char.chars().next() turns '' into None and truncates 'ab' to 'a', which changes predicate behavior instead of returning an input error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-datafusion/src/planner.rs` around lines 797 - 800, Update the
escape handling shared by SQLExpr::ILike and SQLExpr::Like so a
SingleQuotedString is accepted only when it contains exactly one character;
reject empty or multi-character strings with an input error instead of using
char.chars().next() and silently altering the predicate.
Source: Coding guidelines
Comment on lines
+167
to
+169
| RexType::Lambda(_) | RexType::LambdaInvocation(_) => Err(Error::invalid_input( | ||
| "Lambda expressions not allowed in filter expression", | ||
| )), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rust/lance-datafusion/src/substrait.rs"
# Map the file structure first.
ast-grep outline "$FILE" --view expanded
echo
echo "----- relevant slices -----"
sed -n '120,240p' "$FILE"
echo
sed -n '520,620p' "$FILE"
echo
sed -n '840,940p' "$FILE"Repository: lance-format/lance
Length of output: 15547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for lambda-related validation and remapping call sites.
rg -n "Lambda|LambdaInvocation|remap_expr_references|parse_substrait|extension-type|schema length|filter" rust/lance-datafusion/src/substrait.rsRepository: lance-format/lance
Length of output: 4326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rust/lance-datafusion/src/substrait.rs"
# Print only the parse_substrait function and the remap_expr_references helper.
python3 - <<'PY'
from pathlib import Path
path = Path("rust/lance-datafusion/src/substrait.rs")
lines = path.read_text().splitlines()
for start, end, label in [(1, 260, "top"), (520, 640, "middle"), (840, 960, "bottom")]:
print(f"\n===== {label} {start}-{end} =====")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: lance-format/lance
Length of output: 23773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '267,330p' rust/lance-datafusion/src/substrait.rsRepository: lance-format/lance
Length of output: 2559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("rust/lance-datafusion/src/substrait.rs")
for i, line in enumerate(path.read_text().splitlines(), start=1):
if 267 <= i <= 330:
print(f"{i:4d}: {line}")
PYRepository: lance-format/lance
Length of output: 2943
Reject lambda expressions on every parse path. Lambda and LambdaInvocation are only rejected inside remap_expr_references(), so plans that don’t drop any schema fields skip this check and can still reach the consumer. Split validation from remapping, or run the traversal unconditionally, and add a regression test with an unpruned schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-datafusion/src/substrait.rs` around lines 167 - 169, Ensure Lambda
and LambdaInvocation expressions are rejected on every expression-parse path,
not only when schema references are remapped. Update the parsing flow around
remap_expr_references() to traverse and validate expressions unconditionally
while preserving remapping for pruned schemas, and add a regression test using
an unpruned schema that verifies the invalid-input error.
Source: Coding guidelines
Comment on lines
+512
to
+516
| // TODO(datafusion-54): migrate off the deprecated | ||
| // create_aggregate_expr_and_maybe_filter to LoweredAggregateBuilder. | ||
| #[allow(deprecated)] | ||
| fn count_star_expr(input_schema: &SchemaRef) -> Arc<AggregateFunctionExpr> { | ||
| use datafusion::physical_planner::create_aggregate_expr_and_maybe_filter; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the deprecated helper import to the test module’s import block.
Line 516 introduces a use inside count_star_expr, but repository guidelines require imports at the top of the file. Move the import to mod tests and retain deprecation allowances on the import and function call.
Proposed fix
mod tests {
use std::{ops::Bound, sync::Arc};
+ #[allow(deprecated)]
+ use datafusion::physical_planner::create_aggregate_expr_and_maybe_filter;
...
#[allow(deprecated)]
fn count_star_expr(input_schema: &SchemaRef) -> Arc<AggregateFunctionExpr> {
- use datafusion::physical_planner::create_aggregate_expr_and_maybe_filter;
let expr = functions_aggregate::count::count(lit(1));As per coding guidelines: rust/**/*.rs requires use imports at the top of the file, not inline within function bodies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TODO(datafusion-54): migrate off the deprecated | |
| // create_aggregate_expr_and_maybe_filter to LoweredAggregateBuilder. | |
| #[allow(deprecated)] | |
| fn count_star_expr(input_schema: &SchemaRef) -> Arc<AggregateFunctionExpr> { | |
| use datafusion::physical_planner::create_aggregate_expr_and_maybe_filter; | |
| // TODO(datafusion-54): migrate off the deprecated | |
| // create_aggregate_expr_and_maybe_filter to LoweredAggregateBuilder. | |
| #[allow(deprecated)] | |
| fn count_star_expr(input_schema: &SchemaRef) -> Arc<AggregateFunctionExpr> { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/io/exec/count_from_mask.rs` around lines 512 - 516, Move the
create_aggregate_expr_and_maybe_filter import out of count_star_expr and into
the mod tests import block, keeping the deprecation allowance applied to both
the import and its use in the function.
Source: Coding guidelines
Comment on lines
+2057
to
+2060
| let mut df_stats = df_filter_exec | ||
| .partition_statistics(partition)? | ||
| .as_ref() | ||
| .clone(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid unconditional deep copies of Arc-wrapped Statistics.
Use Arc::unwrap_or_clone at both mutation sites so unique allocations are reused. This aligns with the DataFusion 54 migration guidance. (datafusion.apache.org)
rust/lance/src/io/exec/filtered_read.rs#L2057-L2060: replace.as_ref().clone()withArc::unwrap_or_clone(...).rust/lance/src/io/exec/rowids.rs#L291-L291: replace.as_ref().clone()withArc::unwrap_or_clone(...).
📍 Affects 2 files
rust/lance/src/io/exec/filtered_read.rs#L2057-L2060(this comment)rust/lance/src/io/exec/rowids.rs#L291-L291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/io/exec/filtered_read.rs` around lines 2057 - 2060, Replace
the unconditional Statistics clones at both mutation sites with
Arc::unwrap_or_clone so unique Arc allocations are reused: update the
partition_statistics handling in rust/lance/src/io/exec/filtered_read.rs (lines
2057-2060) and the corresponding .as_ref().clone() in
rust/lance/src/io/exec/rowids.rs (line 291).
Source: MCP tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
geodatafusion 0.5.0supports DataFusion 54, which was the last remaining blocker. This bumps DataFusion 53 → 54, and transitivelygeodatafusion0.4 → 0.5,sqlparser0.61 → 0.62, andsubstrait0.62 → 0.63. Arrow stays at 58.Migration for DataFusion 54 breaking changes
as_anywas removed from theExecutionPlan,PhysicalExpr,ScalarUDFImpl,TableProvider,SchemaProvider,CatalogProvider, andCatalogProviderListtraits. The impls are dropped and downcasting now uses the inherentdyn Trait::downcast_ref/is(e.g.plan.downcast_ref::<T>()instead ofplan.as_any().downcast_ref::<T>()).ExecutionPlan::partition_statisticsnow returnsArc<Statistics>.Expr::Cast/TryCasthold aFieldRefinstead of aDataType.AnalyzeExec::newgained ametric_categoriesargument andMetricType::SUMMARYwas renamed toMetricType::Summary.ValueWithSpan.RexType::Lambda/LambdaInvocationvariants and replaces extension URIs with URNs.SimplifyContext::default()builder toSimplifyContext::builder().build().create_aggregate_expr_and_maybe_filteris retained under#[allow(deprecated)]; migrating toLoweredAggregateBuilderis left as follow-up.Verification
cargo clippy --all --tests --benches -- -D warningsclean, plus-p lance-datafusion --features substrait.lance-datafusionsubstrait tests pass (exercising the URN andRexTypechanges).pylance) and Java (lance-jni) bindings compile; all three lockfiles refreshed.🤖 Generated with Claude Code
Summary by CodeRabbit