Skip to content

Optimize Parquet lazy repdef window decode - #784

Open
Gin-Fung wants to merge 3 commits into
bytedance:mainfrom
Gin-Fung:feature/parquet-lazy-repdef-window-decode
Open

Optimize Parquet lazy repdef window decode#784
Gin-Fung wants to merge 3 commits into
bytedance:mainfrom
Gin-Fung:feature/parquet-lazy-repdef-window-decode

Conversation

@Gin-Fung

Copy link
Copy Markdown

Summary

This PR optimizes Parquet repetition/definition level decoding by adding a lazy windowed decode path.

The change is split into three logical commits:

  1. Add option plumbing for Parquet repdef preload window mode.
  2. Add the preload window implementation.
  3. Optimize the hot path by avoiding extra leaf-count decode work and repeated top-level row boundary scans.

Validation

Validated locally with:

  • bolt_dwio_parquet_reader_test
  • bolt_dwio_parquet_table_scan_test
  • bolt_dwio_parquet_cli_test

Both default mode and window preload mode passed.

@CLAassistant

CLAassistant commented Jul 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Gin-Fung
Gin-Fung force-pushed the feature/parquet-lazy-repdef-window-decode branch from 3e62034 to 7389117 Compare July 27, 2026 06:20
void PageReader::decodeRepDefs(int32_t numTopLevelRows) {
if (definitionLevels_.empty() && maxDefine_ > 0) {
preloadRepDefs();
const auto startNs = getCurrentTimeNano();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the core rep/def preload/decode path, but I don't see any tests added in this PR. Can we add coverage for window mode vs the existing path on nested repeated/optional data? The cases I would expect at minimum are: window mode disabled, window mode enabled across multiple pages, and skip/seek after only part of the window has been decoded.

return numRepDefsInPage;
}

auto bitReader = ::arrow::bit_util::BitReader(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a second implementation of Parquet def-level RLE decoding. Can we reuse the existing level decoder path, or extract a shared helper, instead of hand-parsing the RLE runs here?
This is a correctness-sensitive format detail and easy to drift from the normal path.

}

void PageReader::compactConsumedLeafNulls() {
constexpr int32_t WordBits = 64;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the leaf-null compaction logic, including the glibc/FSRM memcpy workaround from the existing path. Can we extract that existing block into a shared helper and call it from both
paths?

void PageReader::preloadRepDefs() {
const auto startNs = getCurrentTimeNano();
hasChunkRepDefs_ = true;
windowRepDefMode_ = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These window-mode fields are tightly coupled. Can we group them into a small private state struct, maybe with a reset() method? That would make the invariants easier to follow.

@liuneng1994

Copy link
Copy Markdown
Collaborator

The optimization direction looks reasonable, but I think this PR needs more work before it is easy to review and maintain.

A few concerns:

  1. Please add checked-in tests for the new windowed rep/def path. Local validation is not enough for this kind of reader state-machine change. We should cover at least: window mode disabled,
    window mode enabled across multiple pages, repeated calls to decodeRepDefs(), and skip/seek after only part of the window has been decoded.

  2. Please add a focused microbenchmark for this optimization. Since this is a performance change, we need benchmark numbers that isolate the new windowed decode path and compare it with the
    existing preload path on representative nested/repeated data.

  3. Some of the new code looks like it is re-implementing existing low-level logic. For example, countLeavesInRepDefPage() hand-parses def-level RLE, and compactConsumedLeafNulls()
    duplicates the existing null compaction logic including the memcpy workaround. Can we reuse the existing level decoding / null compaction path, or extract shared helpers instead of duplicating
    this logic?

  4. The readability could be improved by grouping the new window-mode state. The new window* fields in PageReader are tightly coupled and have reset/compact invariants. A small private
    WindowRepDefState struct with a reset() method would make the state transitions easier to follow.

  5. I would avoid reading BOLT_PARQUET_REPDEF_PRELOAD_WINDOW_COUNT directly inside PageReader. Since the PR already adds RowReaderOptions::setParquetRepDefPreloadWindowCount(), it is
    cleaner to keep this as explicit option/config plumbing rather than a process-wide env var side channel.

Overall, I think the PR should aim for: enough reuse with the existing rep/def decode machinery, tests for the new behavior, and a microbenchmark showing the performance win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants