Skip to content

perf: Optimize fused nested level conversion - #811

Open
liuneng1994 wants to merge 1 commit into
bytedance:mainfrom
liuneng1994:parquet-fused-struct-list-levels
Open

perf: Optimize fused nested level conversion#811
liuneng1994 wants to merge 1 commit into
bytedance:mainfrom
liuneng1994:parquet-fused-struct-list-levels

Conversation

@liuneng1994

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Parquet nested reads currently convert the same DataPage def/rep levels separately when a nullable struct directly wraps a list or map child: once for repeated list/map lengths and validity, and once for the parent struct null bitmap. This repeats the same level traversal on a hot nested-read path.

This PR adds a fused conversion path for the direct nullable struct -> list/map case. The reader now attempts to populate the repeated lengths/list validity and parent struct null bitmap in one pass, then falls back to the existing separate conversions when the level relationship is not supported.

Issue Number: N/A

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 🚀 Performance improvement (optimization)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🔨 Refactoring (no logic changes)
  • 🔧 Build/CI or Infrastructure changes
  • 📝 Documentation only

Description

This PR adds:

  • DefRepLevelsToListLengthsAndStructBitmap, which fuses direct list/map length conversion with direct parent struct bitmap conversion.
  • A PageReader::getListLengthsAndStructNulls wrapper used by nested Parquet readers.
  • Reader-side wiring for direct struct -> list/map children, with fallback to the original separate conversion path when the fused level relationship is not supported.
  • Oracle-based level conversion tests comparing fused output against the existing separate conversions.
  • End-to-end Parquet reader coverage in ParquetReaderTest for continuous reads, non-contiguous skip + next reads, and top-level filtering.
  • Level conversion microbenchmarks for fused fast path and unsupported fallback path.

The fused reader path avoids setting repeated lengths twice by delaying the old repeated setup until the fused path either succeeds or explicitly falls back.

Performance Impact

  • No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

  • Positive Impact: I have run benchmarks.

    Click to view Benchmark Results
    Benchmark: bolt_dwio_parquet_level_conversion_benchmark
    Comparison: separateStructAndList_* vs fusedStructAndList_*
    
    Fast-path fused conversion speedups:
    - single lists: ~1.50x to 1.74x
    - mixed lists:  ~1.85x to 2.17x
    - long lists:   ~1.98x to 1.99x
    
    Unsupported fallback path:
    - fallbackStructAndList_* vs separateStructAndList_* stayed within ~99.0% to 101.1%
    - no systematic regression observed when the fused fast path is not applicable
    
  • Negative Impact: Explained below (e.g., trade-off for correctness).

Release Note

Release Note:

- Optimized Parquet nested level conversion for direct nullable struct -> list/map reads by fusing repeated length and struct null bitmap generation.

Checklist (For Author)

  • I have added/updated unit tests (ctest).
  • I have verified the code with local build (Release/Debug).
  • I have run clang-format / linters.
  • (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
  • No need to test or manual test.

Validation run locally:

cmake --build --preset conan-release --target bolt_dwio_parquet_level_conversion_benchmark bolt_dwio_parquet_arrow_test bolt_dwio_parquet_reader_test --parallel 16

bolt_dwio_parquet_arrow_test --gtest_filter='NestedListTest.*'
bolt_dwio_parquet_reader_test --gtest_filter='ParquetReaderTest.fusedLevelConversion*'
bolt_dwio_parquet_level_conversion_benchmark --bm_regex='(separateStructAndList|fallbackStructAndList)_'
clang-format --dry-run --Werror on touched C++ files
git diff --check

Breaking Changes

  • No

  • Yes (Description: ...)

    Click to view Breaking Changes
    Breaking Changes:
    - N/A
    

Add a fused Parquet level conversion path for direct nullable struct parents over list/map children so the reader can populate repeated lengths and struct nulls from one def/rep traversal.

Add oracle-based level conversion coverage, end-to-end continuous, skipped, and filtered Parquet reads, and fused struct/list cases to the existing level conversion microbenchmark.

Reuse shared level conversion helpers and simplify repeated reader fallback setup.

Co-authored-by: TRAE CLI <noreply@bytedance.com>
RepeatedReader& repeatedReader) {
const auto repDefRange = pageReader.repDefRange();
const int32_t numRepDefs = repDefRange.second - repDefRange.first;
auto lengths = repeatedReader.prepareRepDefLengths(numRepDefs);

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.

Please avoid destructively preparing the output buffers before checking whether the level shape is supported. For a valid but unsupported shape, such as a required list directly under the struct, prepareRepDefLengths() moves the reusable lengths_.lengths() buffer into the local variable, then the fused call returns false and the local buffer is destroyed. The fallback at line 165 can no longer reuse it and must allocate a new buffer on every batch. Please perform the cheap LevelInfo shape check before moving/allocating the outputs, or restore the lengths buffer before returning false.

if (repDefChild == nullptr) {
return nullptr;
}
const bool fuseStructAndRepeated = isArrayOrMap(*repDefChild);

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.

Could we restrict this optimization to nullable structs, e.g. structReader->levelInfo().def_level > 0? For a required struct, the previous setNullsFromRepDefs() path is a no-op, so there are not two conversions to fuse. The new path still allocates and writes an all-valid struct bitmap and installs it as preset nulls, adding work to a path that previously only converted the list.

@guhaiyan0221 guhaiyan0221 changed the title Optimize fused nested level conversion perf: Optimize fused nested level conversion Aug 1, 2026
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.

2 participants