[opt](parquet) accelerate fixed binary decimal decoding - #66379
Merged
Gabriel39 merged 1 commit intoAug 3, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Opinion: approve.
I completed a static review of both changed files at head 57c70283cd35d107bfddc74278d3f0ba7158d020. I found no blocking or non-blocking correctness issue to raise.
- Correctness and portability: the exact-width 4/8/16-byte big-endian loads are equivalent to the prior signed Int256 decoder, while shorter widths retain explicit sign extension. Target-precision validation still precedes every narrowing cast, including the asymmetric signed minimum and whole-source-domain shortcut.
- Lifecycle and compatibility: plain, dictionary, sparse selected-range, BYTE_STREAM_SPLIT, raw-predicate, permissive-null, strict-error, and rollback paths preserve their existing output coordinates and failure semantics. Rescaling, variable BYTE_ARRAY values, and fixed widths above 16 bytes remain on the unchanged Int256 path.
- Tests: the added cases cover the 4-byte target-precision boundaries, permissive null marking, strict rollback over a pre-existing row, shorter negative sign extension, and the complete int32 source domain. Exact 8/16-byte and stateful integration cases are not newly added, but tracing their shared decoder/materialization contracts did not reveal a distinct coverage-driven defect. Per the runner's static-only instruction, I did not rerun builds or tests.
- Performance: the fast path removes per-row Int256 work only for same-scale fixed binary inputs where the narrower carrier is proven safe; the scale-changing and wider fallbacks are unchanged. The reported benchmark was not independently reproduced in this review.
- User focus: no additional focus was provided, so I reviewed the whole PR without narrowing scope.
- Review completion: the main risk scan, two full-coverage reviews, and a separate risk-focused adversarial review all converged on
NO_NEW_VALUABLE_FINDINGS; there were no existing inline threads, proposed inline comments, or unresolved candidates. No repository-local code-review skill or requiredAGENTS.mdfile was present in the review checkout.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Aug 3, 2026
Gabriel39
added a commit
that referenced
this pull request
Aug 4, 2026
…-binary decimal decoding (#66396) ### What problem does this PR solve? Related PR: #66360, #66379 Problem Summary: This PR brings two complementary Parquet V2 optimizations from `branch-4.1` to `master`: 1. reduce predicate-filtering overhead and correctly propagate late runtime filters through the scanner and reader layers; 2. avoid unnecessarily decoding same-scale fixed-binary decimals through `Int256` when their physical width permits a narrower native type. #### Parquet V2 predicate filtering (#66360) - Keep identity selection-vector state implicit and compact selected rows in bulk. - Retain the selection scratch high-water mark across scanner batches and specialize the first compaction from implicit identity. - Refresh late runtime-filter requests at safe row-group boundaries. - Re-run footer-statistics pruning and reset adaptive predicate state for unopened row groups after a refresh. - Preserve real `COUNT(*)` carrier values while runtime filters are pending. - Initialize refreshed JNI predicates and attribute refresh work to TableReader, FileReader, and Parquet profiles. - Preserve Hudi and Paimon child-reader predicate state. - Remove query-scoped dictionary-filter cache state. - Share immutable `VDirectInPredicate` pruning materialization across split-local expression clones. - Add correctness-checked selection-vector and direct-IN lifecycle microbenchmarks. #### Fixed-binary decimal decoding (#66379) - Decode same-scale Parquet `FIXED_LEN_BYTE_ARRAY` decimals with `int32_t`, `int64_t`, or `Int128`, selected from the physical width, instead of always using `Int256`. - Use unaligned full-width big-endian loads for 4-, 8-, and 16-byte values while preserving sign extension for shorter widths. - Validate target precision before narrowing and preserve both permissive and strict conversion behavior. - Keep rescaling and wider values on the existing `Int256` path. - Cover positive and negative precision boundaries, shorter signed inputs, strict rollback, permissive null marking, and the complete int32 source domain. ### Performance The results below come from the original `branch-4.1` PR benchmarks in #66360 and #66379. This PR applies the same implementation to `master`; the performance benchmarks were not re-run as part of this forward-port. #### Selection-vector processing The benchmark validates every surviving original row ID after the timed region. It used the same Clang `-O3 -DNDEBUG -mavx2` benchmark source for the branch base, the pre-fix PR, and the final implementation, with one pinned CPU, three warmups, eight adjacent A-B-B-A quartets, and at least 0.3 seconds per invocation. Negative percentages are improvements. | Operation | Final selectivity | Final vs pre-fix PR | Final vs branch base | | --- | ---: | ---: | ---: | | Identity initialization | 100% | -15.23% | -99.12% | | Row filter | 1% | -24.23% | -23.76% | | Row filter | 50% | -16.10% | -34.50% | | Row filter | 90% | -45.91% | -45.95% | | Row filter | 100% | -31.72% | -17.32% | | Successive filters | 1% | -33.25% | -35.79% | | Successive filters | 50% | -29.80% | -35.10% | | Successive filters | 90% | -25.27% | -25.16% | | Successive filters | 100% | -24.93% | -23.72% | Compared with the branch base, identity initialization improved by 99.12%, row filtering improved by 17.32%-45.95%, and successive filtering improved by 23.72%-35.79%. All final-vs-base paired-ratio CVs were at most 5.85%. #### Direct-IN expression lifecycle `FileScannerExpr/direct_in_clone_prepare_open` isolates deep-clone, prepare, and open for an already prepared direct-IN runtime filter. Set construction and the original fragment prepare/open are outside the timed region. The shared and forced-rematerialization implementations ran in the same Release binary on one pinned CPU, with 10 repetitions and at least 0.5 seconds per repetition. | IN values | Rematerialize median | Shared median | Speedup | | ---: | ---: | ---: | ---: | | 128 | 207.470 us | 1.634 us | 126.9x | | 1,024 | 1.674 ms | 1.642 us | 1,019.5x | | 8,192 | 13.514 ms | 1.672 us | 8,082.2x | | 65,536 | 108.337 ms | 1.650 us | 65,663.1x | The shared path remains approximately constant because split-local clones reuse immutable pruning state instead of rebuilding it for every split. #### Reader-level regression guardrail The Parquet reader benchmark covered nullable INT32 predicate scans with a lazy payload for both PLAIN and dictionary encoding. Across 1%, 10%, 50%, and 90% selectivity, CPU-time changes ranged from -1.34% to +1.41% with mixed signs, so it did not detect a material aggregate reader-level regression. #### Fixed-binary decimal decoding The benchmark decoded 65,536 values per iteration through `DataTypeDecimalSerDe::read_column_from_parquet`, pinned to one CPU, with three warmups followed by 10 repetitions in A-B-B-A order. | Target / physical width | Before median CPU | After median CPU | Speedup | CPU reduction | | --- | ---: | ---: | ---: | ---: | | Decimal32 / 4 bytes | 1,359,514 ns | 88,527 ns | 15.36x | 93.49% | | Decimal64 / 8 bytes | 1,634,757 ns | 90,609 ns | 18.04x | 94.46% | | Decimal128 / 16 bytes | 2,206,272 ns | 152,823 ns | 14.44x | 93.07% | The optimized path was 14.44x-18.04x faster in this benchmark, reducing CPU time by 93.07%-94.46%. The benchmark host was heavily loaded and CPU frequency scaling was enabled, so the exact ratios are noisy; however, the before/after median ranges did not overlap in any A-B-B-A stage. ### Validation on master - 705 filtered ASAN BE unit tests from 53 suites passed, covering Parquet V2, FileScannerV2, TableReader, Hudi/Paimon/JNI readers, SelectionVector, direct-IN pruning, and decimal SerDe. - BE formatting and strict `clang-format` checks passed. - `git diff --check` passed. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason - Behavior changed: - [x] No. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
FIXED_LEN_BYTE_ARRAYdecimals with anint32_t,int64_t, orInt128source selected from the physical width instead of always usingInt256.Int256path.Validation
DataTypeSerDeParquetTestpassed.DataTypeDecimalSerDe::read_column_from_parquet, pinned to one CPU. Each stage used 3 warmups followed by 10 repetitions in ABBA order.The benchmark host was heavily loaded and CPU frequency scaling was enabled, so the exact ratios are noisy. However, the before/after median ranges did not overlap in any ABBA stage. A final optimized-build smoke run measured median CPU times of 95,077 ns, 94,635 ns, and 145,417 ns with CPU CVs of 0.41%, 1.76%, and 0.64%, respectively.