chore(deps): bump datafusion to 55 and arrow to 59 - #37
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // `FilePruner::try_new` itself decides whether a pruner is worth | ||
| // building: it returns `None` for a purely static predicate over a | ||
| // file with no usable column statistics. |
There was a problem hiding this comment.
super nit: the pre-existing sentence a few lines above ("It is assumed that there is no point in doing pruning here if the predicate is not dynamic, as it would have been done at planning time") described exactly the is_dynamic_physical_expr(p) || partitioned_file.has_statistics() guard this commit removes, so it now contradicts this new comment. Worth dropping those two lines so the block reads consistently. (not blocking)
There was a problem hiding this comment.
Good catch — those two lines described the is_dynamic_physical_expr(p) || partitioned_file.has_statistics() guard this PR removed, so they now contradict the comment below them. Dropped them in 7563c2e.
| /// scan stays single-partition and the snapshots stay reproducible. | ||
| pub(super) fn cache_test_config() -> SessionConfig { | ||
| let mut config = SessionConfig::new(); | ||
| config.options_mut().optimizer.repartition_file_min_size = 16 * 1024 * 1024; |
There was a problem hiding this comment.
nit: pinning the threshold makes the snapshots reproducible, but it also means no test now exercises the multi-partition scan path against the shared cache — target_partitions = 4 in create_session_context_with_liquid_cache no longer produces concurrent scan partitions for this file. That concurrency was previously covered incidentally, and a real deployment on a >1 MiB file will hit it by default. Consider adding one test that leaves repartition_file_min_size at the DF 55 default and asserts only order-independent properties (query results, has_cache_hits()), so the concurrent-admission path stays covered without the flakiness. (not blocking)
There was a problem hiding this comment.
Agreed — pinning the threshold did drop coverage of the concurrent multi-partition scan, which is now the default path for any file over 1 MiB. Added test_multi_partition_scan_shares_cache in a7ac25a: it leaves repartition_file_min_size at the DF 55 default, asserts the leaf scan really is multi-partition, and checks only order-independent properties (rows compared as a sorted multiset against a single-partition run, plus has_cache_hits()). The snapshot tests keep their pin.
| let stats = StatisticsContext::new() | ||
| .compute(plan.as_ref(), &StatisticsArgs::new()) | ||
| .unwrap(); |
There was a problem hiding this comment.
super nit: the old call was partition_statistics(None), i.e. explicitly "across all partitions". Worth double-checking that StatisticsArgs::new() defaults to the same thing rather than partition 0 — the admin endpoint would silently start reporting a fraction of the row/byte counts if it doesn't. If it does default to all partitions, ignore this. (not blocking)
There was a problem hiding this comment.
Checked the DF 55 source: StatisticsArgs::new() is all-partitions, so this is equivalent to the old partition_statistics(None). In datafusion-physical-plan-55.0.0/src/statistics.rs, StatisticsArgs is #[derive(Debug, Default, Clone)] pub struct StatisticsArgs { partition: Option<usize> } (L73-76) and pub fn new() -> Self { Default::default() } (L83-85), i.e. partition: None; its doc comment states "By default the partition is set to None (statistics should be computed for the entire plan)" and set_partition documents None as "statistics should be computed for the entire plan". StatisticsContext::compute only restricts to a partition when args.partition() is Some(idx) (L159-168). Upstream itself uses this exact call as the partition_statistics(None) equivalent — see operator_statistics/mod.rs L358-362, where StatisticsContext::new().compute(plan, &StatisticsArgs::new()) is described as partition_statistics(None) with no overhead. No change needed.
There was a problem hiding this comment.
The full diff was omitted from the review context, so I fetched it with gh pr diff and read the post-merge files for src/datafusion/src/reader/plantime/source.rs, opener.rs, src/datafusion/src/optimizers/mod.rs, src/datafusion-client/src/client_exec.rs, optimizer.rs, src/datafusion-local/src/lib.rs, src/datafusion-local/src/tests/mod.rs, and src/datafusion-server/src/admin_server/handlers.rs.
The API adaptations look like faithful ports: replace_children_if_necessary matches the hand-rolled ptr-eq logic it replaces, PruningPredicateBuilder is passed the same schema the old PruningPredicate::try_new got (and PrunableStatistics still uses the same one), and LiquidCacheClientExec::apply_expressions is correct given the node holds no expressions of its own and exposes remote_plan as a child. LiquidParquetSource::apply_expressions covers both predicate and projection, which are the only expression-bearing fields on the struct. The TPC-H snapshot churn is DF 55 display changes (decimal rendering, join_type line) plus planner sort/projection reordering — no structural change at the LiquidCacheClientExec pushdown boundary.
The cache_test_config workaround is well reasoned and applied to every test that reads nano_hits.parquet and asserts order-sensitive cache state; src/datafusion-server/src/tests/mod.rs also uses that file but only asserts query results, so it doesn't need it.
CI checks were all still queued or in progress when this review started, so I have no test results to report either way. Three non-blocking comments inline.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
There was a problem hiding this comment.
Cycle 2. The ## Full diff block was omitted as too large, so I fetched the patch with gh pr diff 37 and read the post-change files directly: src/datafusion/src/reader/plantime/source.rs, src/datafusion/src/reader/plantime/opener.rs, src/datafusion-client/src/client_exec.rs, src/datafusion-local/src/tests/mod.rs, src/datafusion-local/src/tests/{squeeze,filter_limit,page_index}.rs, src/datafusion-server/src/admin_server/handlers.rs, and the two datafusion-local snapshots.
The since-diff (provide_schema2.snap) moves SortExec below the ProjectionExec onto the raw date_bin group key. That is a 1:1 projection above the sort, so the SortPreservingMergeExec on zo_sql_key still sees the same order — plan-shape refresh only.
Spot-checked the rest of the port: LiquidParquetSource::apply_expressions covers both expression-holding fields (predicate, projection); LiquidCacheClientExec genuinely holds none of its own; replace_children_if_necessary reproduces the old ptr-eq short circuit; the PruningPredicateBuilder conversions keep the same schema. cache_test_config() is applied to every test that reads the 2.3 MB nano_hits.parquet — the other SessionConfig::new() tests write their own small files, so they stay single-partition regardless.
No blocking issues. The three prior threads were all non-blocking and I am not re-raising them. CI checks were still queued or in progress at review time, so I am not asserting anything about test results.
There was a problem hiding this comment.
The full diff was omitted from my review context, so I fetched it with gh pr diff 37 and read the non-lockfile portion in full, plus the surrounding code in src/datafusion-local/src/tests/mod.rs, src/core/src/cache/core.rs, src/core/src/cache/observer/mod.rs, src/datafusion-client/src/client_exec.rs, src/datafusion/src/optimizers/mod.rs, and src/datafusion/src/reader/plantime/source.rs from the checkout.
Both prior nits are addressed: the stale pruning comment is gone in 7563c2e, and test_multi_partition_scan_shares_cache in a7ac25a covers the default repartition_file_min_size path with order-independent assertions only. The cache.storage().stats() counter-reset idiom it uses is correct — LiquidCache::stats goes through Observer::runtime_snapshot, which consumes and zeroes the runtime counters.
No blocking issues. Note that CI was still queued/in progress when this review ran, so I have not seen results for the new test or the refreshed snapshots.
1 participant
Moves the workspace to DataFusion 55 and the arrow/parquet family to 59.2, adapting to the required API changes (
FileSource/ExecutionPlan::apply_expressions,ConfigNonZeroUsizebatch size,PruningPredicateBuilder,StatisticsContext,replace_children_if_necessary,VariantArray::typed_value_column).DataFusion 55 also lowers the
repartition_file_min_sizedefault from 10 MiB to 1 MiB, which splits the 2.3 MB test file across scan partitions and made the cache-trace snapshots depend on scheduling and core count; the affected tests now pin the threshold above the file size.