lucasfang commented on PR #314:
URL: https://github.com/apache/paimon-cpp/pull/314#issuecomment-5678135996
# Review: PR #314 — perf(parquet): reuse reader-local offset indexes and
page plans
Thanks for narrowing this down. Correctness looks sound across all three
changes; my concern is benefit vs. complexity, and I'd suggest splitting rather
than merging as-is.
## Correctness (verified)
- `VisitSelectedPages` invariants hold: `RowRanges` guarantees ascending,
disjoint, inclusive ranges (`Add`/`Union`/`Intersection` all maintain
sorted-disjoint), and `first_row_index` is strictly increasing. The `next_page
= first` advance dedupes pages shared by several ranges, so each selected page
is visited exactly once — which is what both `MakeDataPageReadPlan` (no
duplicate entries) and `ComputePageRanges` (no duplicate ranges) need.
- `direct_read_columns` is semantically equivalent to the old `offset_index
&& MakeDataPageReadPlan(...)` gate. The factory runs during `GetColumn`, before
the leaf loop, and `MakeDataPageReadPlan` is a pure function of inputs that are
now identical (memoized `GetOffsetIndex` returns the same object), so dropping
the second validation is safe.
- `CachedRowGroupPageIndexReader` caches `nullptr` for missing indexes
correctly, and `GetOffsetIndex(-1)` throws before `emplace`, so the cache is
not polluted.
## The part worth keeping
OffsetIndex memoization is the only change with a demonstrated CPU benefit.
Within one file reader, `GetOffsetIndex` is called from several sites for the
same (row group, column) — `column_index_filter`, the
factory/leaf-loop/`ComputePageRanges` in `page_filtered_row_group_reader`, and
`parquet_file_batch_reader` — all sharing the same cached wrapper via
`GetRowGroupPageIndexReader`. Arrow 17 re-runs `OffsetIndex::Make` (O(pages))
on every lookup, so memoizing removes real repeated deserialization. The
`direct_read_columns` reuse (removing a duplicate `MakeDataPageReadPlan`) is a
low-risk companion to this.
## Concerns
- Performance is not established. The PR's own benchmark table is within
noise, with a regression in the first 10% case, and the description states "No
general throughput gain is established." These are timings, not CPU profiles,
and the isolated high-page-count measurement is still outstanding.
- `VisitSelectedPages` has poor benefit/complexity. This path already makes
three linear passes over the same page vector: `GetDataPageLayout` validates
every page, `VisitSelectedPages` selects, and `ComputeCompressedRowRanges`
walks every page again. The cursor+binary-search removes one of three passes,
so the overall cost stays O(pages). The cost is a helper that is only correct
while ranges stay ascending/disjoint and `first_row_index` strictly increasing
— an invariant carried by a comment callers must honor — plus a 121-line
equivalence test. I'd drop this until a profile shows the selection scan is a
real hotspot.
- Memory: after memoization, each retained row-group reader holds parsed
OffsetIndex objects, up to the 1,024-reader limit. As the doc notes, that is a
count limit, not a byte budget; for many-row-group, many-page files the
resident parsed objects can be significantly larger than the prior byte cache.
Bounded, but worth an explicit magnitude note or a soft cap on very large page
indexes.
- Two tests add little:
- `ParsedOffsetIndexesRespectRowGroupRetentionLimit` writes 1,025
single-row row groups to re-assert `kMaxRowGroupPageIndexReaders`, a constant
this PR does not change, and pins it via `weak_ptr` expiry — the retention
map's internals, not the new memoization.
- `MissingPageIndexesRemainReadable`: with `enable_page_index=false`,
`RowGroup()` returns nullptr, so the entire `if (indexes)` assertion body is
skipped, leaving a 2,000-row read already covered elsewhere.
- `ReusesParsedOffsetIndexesWithinFileReader` is the one that covers the
actual change; keep it.
## Suggested resolution
- Keep: OffsetIndex memoization (`CachedRowGroupPageIndexReader`), the
`direct_read_columns` reuse, `ReusesParsedOffsetIndexesWithinFileReader`, and
the doc update.
- Drop (or back with a high-page-count CPU profile): the
`VisitSelectedPages` page-cursor rewrite and its 121-line equivalence test.
- Drop: `ParsedOffsetIndexesRespectRowGroupRetentionLimit` and
`MissingPageIndexesRemainReadable`.
- If memoization is kept, add an explicit note (or a soft cap) on the 1,024
x parsed-index memory upper bound.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
