[improvement](parquet) Vectorize File Scanner V2 decode hot paths#65972
[improvement](parquet) Vectorize File Scanner V2 decode hot paths#65972Gabriel39 wants to merge 2 commits into
Conversation
Add runtime-dispatched AVX2 kernels with scalar fallbacks for byte-stream-split transpose, delta prefix sums, numeric dictionary gather, nullable expansion, and raw predicate comparisons. Wire the kernels into File Scanner V2 and extend the Parquet benchmark matrix with 80 fixed-width kernel scenarios.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for three supported-build blockers in the new SIMD translation unit and one benchmark validation/documentation gap.
Critical-checkpoint conclusions:
- Goal and scope: the patch is focused on vectorizing five File Scanner V2 decode hot paths and leaves V1 unchanged, but it is not portable across Doris's supported build matrix yet.
- Concurrency, lifecycle, and memory safety: no new threads, locks, shared mutable state, ownership cycles, or non-intuitive resource lifecycles were introduced. The immutable lookup tables, runtime CPU check, dictionary materialization, COW boundary, and rollback paths were reviewed without another substantiated issue.
- Error handling, data correctness, and nullable behavior: decode statuses, cursor advancement, dictionary-ID validation, rollback, byte-stream-split ordering, delta carry propagation, raw-comparison masks/NaNs, and nullable expansion were traced. No additional correctness defect remained after resolving the nullable-payload candidate against the exact pre-patch constructors and downstream null replacement.
- Configuration, compatibility, and parallel paths: there are no new configs, persisted formats, protocol symbols, FE-BE variables, transactions, or data-write semantics. The AVX2, baseline-x86, and non-x86 paths were all reviewed; the inline P1 comments identify failures in the latter two supported configurations.
- Conditions, tests, and results: the width/threshold/tail conditions and focused SIMD tests are generally appropriate, but there is no coverage for the failing non-x86, no-PCH, or
USE_AVX2=0compilation modes. The expanded decoder cases also lack an untimed result oracle and leave the mandatory registration-count guide stale. No generated result files changed. - Observability and performance: no new production logging or metrics are required for these local kernels. The microbenchmarks improve coverage, but the PR provides no controlled before/after comparison, so no unmeasured speedup or scalar regression was treated as established.
Validation was static as required by the review task; no local build or test was run, and this checkout lacks .worktree_initialized, thirdparty/installed, and protoc. At submission time, CI compile/style checks passed, the performance job failed, and several test/regression jobs were still pending. No additional user focus was supplied. All 16 changed files were swept, all candidates were resolved, and the production, benchmark, and risk reviewers converged on this four-comment set.
| namespace doris::parquet_simd { | ||
| namespace { | ||
|
|
||
| bool has_avx2() { |
There was a problem hiding this comment.
[P1] Guard this helper on non-x86 builds
When DORIS_PARQUET_X86_SIMD is absent, every call to has_avx2() is compiled out, but this anonymous-namespace definition remains. Doris builds ARM with -Wall -Wextra -Werror (and Clang enables -Wunused), so the unused internal function is a build error before the scalar fallback can run. Please put the helper itself under the x86 guard (or otherwise remove the non-x86 definition) and cover an arm64/AArch64 compile.
| for (size_t lane = 0; lane < 8; ++lane) { | ||
| valid_mask |= static_cast<uint32_t>(nulls[start + lane] == 0) << lane; | ||
| } | ||
| const size_t valid = std::popcount(valid_mask); |
There was a problem hiding this comment.
[P1] Include the header that declares std::popcount
This translation unit uses std::popcount here and at line 237 but never includes <bit>. GCC builds explicitly default to ENABLE_PCH=OFF, so a transitive declaration from Clang's PCH cannot make this a supported build contract. Please add a direct #include <bit> and validate the no-PCH configuration.
| } | ||
|
|
||
| template <typename Vec> | ||
| Vec combine_comparison(Vec equal, Vec greater, Vec less, RawComparisonOp op) { |
There was a problem hiding this comment.
[P1] Keep every AVX2 intrinsic helper in an AVX2 target scope
combine_comparison (including its generated lambda call operators) directly invokes _mm256_* intrinsics and returns 256-bit vectors, but it has no target("avx2") scope. The attribute on raw_compare_*_avx2 does not propagate to a separately instantiated helper, so the supported USE_AVX2=0 x86 build encounters AVX ABI/always-inline target-mismatch diagnostics before runtime dispatch. Please inline this selection into the annotated functions or place the complete helper implementation in an AVX2 target scope, then compile-check USE_AVX2=0.
| inline bool register_decoder_benchmarks() { | ||
| for (const auto& scenario : decoder_scenarios()) { | ||
| for (const int selectivity : {1, 10, 50, 100}) { | ||
| for (const int selectivity : {0, 1, 10, 50, 90, 100}) { |
There was a problem hiding this comment.
[P2] Make the new decoder boundary cases verifiable
This loop now registers 19 * 6 * 2 = 228 decoder cases, plus 80 new kernel cases, while the mandatory be/benchmark/parquet/AGENTS.md still tells maintainers to expect 152 and omits ParquetKernel. Also, run_decoder only resets each sink's consumed field; it never checks that field against plan.selected_rows or validates representative decoded values outside timing. Consequently sel_0 can emit rows, or sel_90 can return the wrong selection/payload, and still publish a successful benchmark. Please update the current count guidance and add an untimed count/checksum oracle before measuring these new cases.
### What problem does this PR solve? Issue Number: None Related PR: apache#65972 Problem Summary: GCC 15 rejected the vector comparison helper because it inspected attributed AVX vector types through std::is_same, baseline and non-x86 builds did not keep every architecture-specific helper in a valid scope, and the expanded decoder benchmark matrix did not verify selected output. Keep all AVX2 operations in explicitly targeted helpers, keep scalar comparison available on non-x86 builds, include the direct popcount dependency, move the reusable kernels to the doris::simd namespace, and validate decoder counts plus deterministic value checksums outside the timed region. Update the documented and tested benchmark registration counts. ### Release note None ### Check List (For Author) - Test: Unit Test and Manual test - Parquet SIMD kernel unit tests: 5 passed - Parquet benchmark scenario unit tests: 7 passed - GCC 15 AVX2, baseline x86 without AVX2, and scalar-only AArch64-emulated compile checks - Release benchmark target compiled and linked; 228 decoder and 80 kernel smoke cases completed without errors - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for one remaining BYTE_STREAM_SPLIT fallback-performance regression.
Critical-checkpoint conclusions:
- Goal, scope, and proof: the patch coherently adds runtime-dispatched kernels for five File Scanner V2 hot paths and leaves V1 unchanged. The functional SIMD/scalar contracts are generally preserved, but the BSS integration does not preserve the existing generic path for unsupported widths, short batches, and non-AVX2 CPUs as the PR states.
- Size and clarity: the shared kernel abstraction and benchmark additions are understandable, but replacing the established BSS scalar decoder is an avoidable expansion of the optimization's scope; the inline P2 identifies the remaining change needed.
- Concurrency, lifecycle, and memory safety: these kernels introduce no threads, locks, shared mutable state, cross-TU initializer dependency, ownership cycle, or special resource lifecycle. Dictionary COW/materialization and late-error rollback were traced without another finding.
- Data correctness, error handling, and conditions: BSS coordinates/tails, delta carry and wrapping arithmetic, dictionary-ID validation, nullable in-place expansion, raw-predicate operand reversal, masks, NaNs, and pre-consumption fallback behavior were checked. No data-correctness issue remains. The TIMESTAMPTZ NULL-payload candidate was dismissed because nullable access is null-map-gated and validity-sensitive consumers replace NULL payloads with the type default before evaluation.
- Configuration, compatibility, and parallel paths: no configuration, persisted format, protocol, function symbol, FE-BE variable, transaction, or data-write behavior changes. File Scanner V1 is unchanged. The supported arm64, baseline-x86, unsupported-width, and short-batch BSS paths are precisely where the inline performance issue applies.
- Tests and results: the 228 decoder, 80 kernel, and 151 reader registrations, decoder checksums, setup timing, and mutable-input resets are coherent. Current-head commit metadata records seven scenario tests although the PR body still says 6/6. Existing tests cover scalar correctness for a 31-value 4/8-byte case, but there is no forced-scalar before/after guard for widths 1/2/4/8/12/16 or arbitrary fixed lengths, so the fallback replacement is not performance-protected.
- Observability: no additional production logging or metrics are needed for these local kernels.
- Performance: the inline P2 is implementation-based rather than an inferred speedup claim: production now reaches a row-major byte loop where the existing decoder provides width-1 memcpy and an explicitly tuned blocked scalar implementation.
Validation was static as required by the review task; no local Doris build or test was run. This checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc. At submission time, compile, Cloud UT, FE UT, formatting, style, license, and secret checks passed; coverage and performance checks failed; BE UT, macOS BE UT, code review, and several regression checks remained pending. No additional user-provided review focus was supplied. All 17 changed files were swept, every candidate was adjudicated, and three review rounds converged on this single non-duplicate comment.
| return; | ||
| } | ||
| #endif | ||
| byte_stream_split_decode_scalar(src, width, offset, num_values, stride, dest); |
There was a problem hiding this comment.
[P2] Keep the optimized BYTE_STREAM_SPLIT scalar fallback
Both production BYTE_STREAM_SPLIT read paths now enter this dispatcher for every width, but AVX2 only handles 4/8-byte batches with at least 32 rows. Every other case reaches this byte-at-a-time fallback, replacing the existing width-1 bulk memcpy and the explicitly performance-tuned 128-row scalar implementation for widths 2/4/8/16 and dynamic widths. That regresses supported INT96 and arbitrary-width FIXED_LEN_BYTE_ARRAY decodes, short 4/8-byte batches, and all reads on arm64 or baseline x86—the exact fallback configurations the PR says remain preserved. Call the existing optimized decoder here (or retain an equivalent implementation), and add forced-scalar before/after coverage for the supported widths.
2 participants
What problem does this PR solve?
File Scanner V2 still executes several fixed-width Parquet decode and filter stages with scalar row loops. On hot scans, byte-stream-split transpose, DELTA_BINARY_PACKED reconstruction, numeric dictionary materialization, nullable expansion, and raw predicate comparison can become CPU bottlenecks after I/O is cached.
What is changed?
ColumnChunkReadernullable POD expansionNaN == NaN, and NaN sorts above finite values).ParquetKernelscenariosFile Scanner V1 is unchanged.
Check list
ParquetSimdKernelsTest: 5/5 passedParquetBenchmarkScenariosTest: 6/6 passedCore/Exprs/Format/Utilstatic libraries compiledbenchmark_main.cppcompiled with Doris project flags and-Werrorgit diff --checkThe local full benchmark build was not used for performance claims because the host's broken
git-submodulepyenv shim forced an unrelated 14k-target OpenBLAS rebuild. Stable before/after performance comparison should use the checked-in matrix on a controlled host.