feat(query): make lazy row fetch page-aware#20166
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d52b27f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .with_offset_index_policy(PageIndexPolicy::Required) | ||
| .load_and_finish(&mut file_reader, part.file_size) | ||
| .await?; |
There was a problem hiding this comment.
Fall back when offset indexes are absent
When lazy RowFetch reaches a block without a Parquet OffsetIndex, this PageIndexPolicy::Required load returns an error and the ? propagates it, so the code never reaches the intended metadata.offset_index().is_none() fallback. This affects data_page_rows tables written by the current BlockParquetWriter path, which the repo documents as omitting OffsetIndex for multi-page chunks, causing qualifying lazy RowFetch queries to fail instead of using the existing full-block path; use an optional policy or handle the missing-index error as Ok(None).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2a5fd58f25: the metadata reader now uses PageIndexPolicy::Optional. Files without an OffsetIndex return None from the page-aware path and fall back to the existing full-block RowFetch path. Validation on the rebased PR head: fuse check passed, fuse lib tests 89/89, cache test 1/1, and clippy with -D warnings passed.
|
CI fixture failure fixed in
The PR head is now |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Extract the page-aware lazy RowFetch changes from #20144 into an independently reviewable storage PR.
The existing generic lazy plan scans filter/order columns plus
_row_id, appliesSortandLimit, then fetches the remaining projected columns. Before this PR, each hit block's projected columns were read and decoded in full. This PR maps the selected row ids to their Parquet page range and reads only that range before taking the requested rows.This implementation does not depend on ordered top-k,
Sort(PresortedMerge),PagePruner, composite/prefix page statistics, a planner cache, or a query-result cache. The captured lazy plans remainRowFetch -> Limit -> Sort(Single) -> TableScanand contain noPresortedMerge.Implementation
data_page_rowsoption.DataBlockentries.lazy_read_threshold=0deliberately bypasses RowFetch, so this optimization is expected to be neutral in no-lazy mode.Benchmark
Driver: https://github.com/dbsid/hbx-web3
Environment and methodology:
i4i.4xlarge, 16 vCPU Intel Xeon Platinum 8375C, single query node, S3 inus-west-2;RUSTFLAGS="-C target-cpu=native";max_threads=2,max_storage_io_requests=16, distributed pruning off;2026071305;lazy_read_threshold=1000and0;enable_query_result_cache=0; the pre-existing mainlineenable_planner_cache=1is identical for baseline and PR.Binaries:
7dc41477f7: SHA256f3d3766575b1b00dce4ccb37597a4a271d0727bfb5b12e97d09ceb930a74899f;643c2fc22b: SHA256cc32a5abdee80b193ef61aa2b5a5c045e9ebd0097777add433ff7d29864883e9;77f587e268ea4147dc9235f6101443ea51e9ee7a7c9528de9bb0e0f3a4e5a06f.Authoritative round-2 results:
With generic lazy RowFetch enabled, mean latency improves by 75.09%-76.80% (4.01x-4.31x) at 5 connections and by 70.88%-72.18% (3.43x-3.59x) at 10 connections. With lazy reads disabled, mean changes range from -0.68% to +0.75%, consistent with benchmark noise and the intentional no-lazy bypass.
These results isolate page-aware RowFetch from
PresortedMerge: the branch contains no ordered-top-k planner/executor changes, all lazy plans containRowFetch -> Limit -> Sort(Single) -> TableScan, and all no-lazy plans omitRowFetch; none containPresortedMerge.Correctness
lazy_read_threshold=0and1000passed Q1 200/200, Q2 200/200, and Flex 200/200, with zero mismatches and zero errors.RowFetchoverLimit -> Sort(Single) -> TableScan; no-lazy plans containLimit -> Sort(Single) -> TableScan.Validation
cargo fmt --all -- --checkgit diff --checkcargo check -p databend-common-storages-fusecargo test -p databend-common-storages-fuse range --lib(6 passed)cargo test -p databend-common-storages-fuse --lib(90 passed)cargo test -p databend-storages-common-cache test_table_data_cache_mixed_entries --lib(1 passed)cargo clippy -p databend-common-storages-fuse --lib --tests -- -D warningsbash -n tests/task/test-private-task.shdatabend-querybuild withRUSTFLAGS="-C target-cpu=native"Review follow-up
The initial review found that
PageIndexPolicy::Requiredcaused lazy RowFetch to fail on Parquet files without an OffsetIndex before the documented full-block fallback could run. Commit2a5fd58f25changes the metadata read toPageIndexPolicy::Optional; missing indexes now returnNonefrom the page-aware path and use the existing full-block RowFetch implementation.A second review found that the process-wide projected-block metadata cache key used
table_id/seq, although time-travelFuseTableinstances documentident.seqas a placeholder. Commit1d9d44e295adds operator-root and snapshot identity to the key, with regression coverage for snapshot and storage isolation.The final deep review found two remaining cache-safety gaps: table identity/sequence and
data_page_rowswere not both represented in the metadata key, and failure to construct a collision-safe page-range data key did not force the documented full-block fallback. Commit0b507412a3adds all invalidation dimensions and makes an unavailable range key returnNoneimmediately so the existing full-block path runs.CI also exposed a race in the private-task integration test: its second fan-out execution could begin before the first three child runs reached a terminal state. Commit
643c2fc22bwaits for all first-run children to becomeSUCCEEDEDbefore mutating the root run and starting the second execution.Tests
Type of change
This change is