Adopt upstream DataFusion 54 + size-cap patch#7
Merged
Conversation
some nice refactoring to move things around
This is one of the most important yet very complicated refactor. Many things can go wrong, especially around the datafusion integration: concurrency, proper filter/projection/schema etc. This is the first phase of the 2, where we stop taking new entries when the cache is full (e.g. used up disk space). The second phase will allow properly evicting entries from disk.
When refactoring things, a few bugs surfaced. Fixing them.
…engHao#497) This is a major refactoring of the liquid stream, so that it does not rely on the invariant that entires must be in the cache at streaming time. Instead, it will first try to read from liquid cache, and if non-exists, it will build parquet stream and read from parquet.
When disk is full, we will run replacement to evict old items. This pr also clean up the unnecessary policies (they don't really makes sense)
With the latest t4, we will have real deletion and real disk replacement
## Release v0.1.13 This PR bumps all workspace crate versions to `0.1.13`. --- **On merge**, the `publish.yml` workflow will automatically: 1. Publish all crates to crates.io 2. Create a GitHub Release with tag `v0.1.13` Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…XiangpengHao#504) CachedMetaReaderFactory::create_liquid_reader constructed its inner ParquetObjectReader without the known file size, forcing parquet 58.x onto the suffix-fetch metadata path. With a large metadata_size_hint and a file containing page indexes, that path can slice page-index ranges against a retained suffix using the wrong base offset, panicking with `assertion failed: end <= remainder.len()`. Forward the already-known object_meta.size via with_file_size() so metadata loading uses the size-aware bounded-range path, matching DataFusion's parquet reader factories. Fixes XiangpengHao#503
Remove the hack
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+70
to
+78
| let mut convert = |node: &Arc<dyn ExecutionPlan>, hints: ColumnSqueezeHints| { | ||
| // Leave oversized scans as vanilla parquet reads, bypassing the cache. | ||
| if let Some(max_bytes) = max_scan_bytes | ||
| && parquet_scan_total_bytes(node).is_some_and(|total| total > max_bytes) | ||
| { | ||
| return None; | ||
| } | ||
| convert_parquet_scan(node, &cache, hints) | ||
| }; |
There was a problem hiding this comment.
nit: the size-cap pass-through is the sole hotdata-specific behavior in this PR, but it has no test coverage. The existing test_plan_rewrite exercises only the default (unbounded) path, so a regression that stops honoring max_scan_bytes — or that mis-computes parquet_scan_total_bytes — would go unnoticed. Consider a test that registers a parquet table, sets with_max_scan_bytes below the file size, and asserts the scan is left as a plain ParquetSource (not wrapped in LiquidParquetSource), plus the inverse above-threshold case. (not blocking)
There was a problem hiding this comment.
Reviewed the hotdata-specific max_scan_bytes patch on top of the DataFusion 54 upgrade. The pass-through logic is correct: returning None from the convert closure leaves oversized parquet scans as vanilla DataFusion reads. One non-blocking nit left inline about missing test coverage for the new feature.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Sync the fork onto upstream XiangpengHao/liquid-cache@746d639 (DataFusion 54), discarding the stale df53 fork commits. The page-index/file_size fix is now upstream (XiangpengHao#504) and max_disk_bytes is also upstream, so the fork's old carries are dropped. The hotdata size-cap patch is re-applied on top in the following commit.
LocalModeOptimizer gains an optional max_scan_bytes; when set, parquet scans whose total file size exceeds the threshold are left as vanilla DataFusion reads instead of being wrapped by LiquidCache. Exposed via LiquidCacheLocalBuilder::with_max_scan_bytes. Ported onto the DataFusion 54 optimizer API (convert closure + squeeze hints), and covered by a test asserting the above/below-threshold split.
anoop-narang
force-pushed
the
adopt-datafusion-54
branch
from
bdc52b1 to
0ed51a8
Compare
There was a problem hiding this comment.
Reviewed the single hotdata-specific change (size-cap pass-through). The builder→optimizer wiring is correct, and the prior test-coverage gap is now closed by test_max_scan_bytes_pass_through, which asserts both the below- and above-threshold cases. Rest of the diff is the upstream DataFusion 54 adoption. LGTM.
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
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
3 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.
Brings the fork's
mainup to upstreamXiangpengHao/liquid-cache@746d639(DataFusion 54) and re-applies the single hotdata patch on top.What lands
746d639), replacing the stale df53 base.feat: table-size based cache pass-through(with_max_scan_bytesonLocalModeOptimizer+LiquidCacheLocalBuilder; parquet scans over the threshold bypass the cache), ported to df54's refactored optimizer API.What's dropped (no longer needed)
file_sizefix — now upstream as Propagate file_size to ParquetObjectReader in CachedMetaReaderFactory XiangpengHao/liquid-cache#504.max_disk_bytes— also upstream.Mechanics
This is a fast-forwardable merge commit (parents = current
main+ the ported df54 branch; tree = the df54+patch state), somainadvances with no force-push, satisfying branch protection. The old df53mainis archived at tagpre-df54-df53-main.Verification
cargo fmt --check+clippyclean on the touched crates.Part of the hotdata-wide DataFusion 53→54 upgrade (runtimedb #886).