Uh oh!
There was an error while loading. Please reload this page.
[improvement](parquet) Lazily materialize complex residual columns - #65965
Conversation
hello-stephen
commented
Jul 23, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
commented
Jul 23, 2026
run buildall |
Gabriel39
commented
Jul 23, 2026
/review |
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 28th, 2026 5:08 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
hello-stephen
commented
Jul 23, 2026
TPC-H: Total hot run time: 29295 ms |
hello-stephen
commented
Jul 23, 2026
TPC-DS: Total hot run time: 177005 ms |
hello-stephen
commented
Jul 23, 2026
ClickBench: Total hot run time: 24.91 s |
bbbdeb1 to
913663eCompareGabriel39
commented
Jul 23, 2026
run buildall |
Gabriel39
commented
Jul 23, 2026
/review |
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 28th, 2026 5:08 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
hello-stephen
commented
Jul 23, 2026
TPC-H: Total hot run time: 29235 ms |
hello-stephen
commented
Jul 23, 2026
TPC-DS: Total hot run time: 178100 ms |
hello-stephen
commented
Jul 23, 2026
ClickBench: Total hot run time: 25.02 s |
hello-stephen
commented
Jul 23, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
commented
Jul 23, 2026
/review |
hello-stephen
commented
Jul 23, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Review status: incomplete formal convergence. The third and final permitted subagent round found the localized-CAST issue below, so the review did not reach an all-NO_NEW_VALUABLE_FINDINGS round. The three-round cap is exhausted; every candidate currently known has nevertheless been independently checked, deduplicated, and resolved in this review.
Checkpoint conclusions:
- Goal and data correctness: staged lazy materialization is coherent for expressions that are correctly classified as selected-row safe, and the selection/cursor/delete invariants hold on the paths inspected. However, localized ordinary CAST nodes are misclassified and can now suppress a conversion error; see the inline request.
- Scope and focus: all eight changed files and the relevant expression, ColumnMapper, reader, delete, and benchmark paths were reviewed. No additional user focus was provided.
- Concurrency, lifecycle, and memory: scheduler state remains serial and request/row-group scoped; retained expression owners and reset behavior are sound, and the new scratch structures are bounded by predicate count.
- Configuration, compatibility, protocol, persistence, transactions, and data writes: no such contracts are changed by this read-only Parquet scheduling patch.
- Parallel paths and conditions: safe versus eager fallback, nested AND/OR, constants and nullable WHERE behavior, dense/select/skip cursor consumption, dictionary and direct filtering, virtual columns, position/equality deletes, empty batches, pending output, page/batch boundaries, and Row Group transitions were traced. No additional correctness defect was substantiated.
- Tests and proof: the new scanner tests validate the bespoke two-stage harness, but do not prove the documented production arithmetic expression, an all-pass long chain, or a localized failing cast. The scenario matrix also does not assert its advertised total. No local build or test was run because the governing review-runner contract prohibits it. Current CI reports compile, BE/macOS UT, regression, formatter/style, coverage, secrets, and performance checks passing; the automated code-review check remains pending.
- Error handling and observability: Status propagation is otherwise preserved. Existing counters expose reads/selects/skips and actual compaction, but not repeated no-op mapping walks.
- Performance: the per-stage alignment loop is quadratic for high-survival chains. Separately, the PR's reported 2.50% improvement is inconclusive under the benchmark guide because the run used a high-load, CPU-scaling-enabled host, paired medians drift roughly 55%-61%, and no variability/confidence evidence is reported.
Please address the four inline issues before merge.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
913663e to
8eb8cdbCompareGabriel39
commented
Jul 23, 2026
run buildall |
Gabriel39
commented
Jul 23, 2026
/review |
hello-stephen
commented
Jul 23, 2026
TPC-H: Total hot run time: 29391 ms |
hello-stephen
commented
Jul 23, 2026
TPC-DS: Total hot run time: 177149 ms |
There was a problem hiding this comment.
Review result: request changes. Two new patch-scoped issues remain: staged residual decoding can suppress a strict-mode native conversion error, and the new complex-residual benchmark measures duplicate expression teardown.
Critical checkpoint conclusions:
- Goal and proof: the patch does lazily materialize later residual columns for valid safe-AND workloads, and the added scan/profile tests exercise the intended staged path. It does not yet preserve the prior strict-cast error result for invalid later-column physical values; see the P1 inline comment.
- Scope and focus: the implementation is generally focused across scheduler, profile, benchmark, documentation, and tests. No additional user-provided review focus was supplied.
- Concurrency: no new shared cross-thread state, locks, atomics, or lock ordering are introduced; the new schedule, selection, and profile state are reader-scoped.
- Lifecycle and static initialization: no cross-TU static-order issue was found. The benchmark's new expression ownership has a lifecycle defect: ReaderSession teardown happens after timing resumes and the context remains request-owned, producing the P2 inline comment.
- Configuration: no configuration item is added. The P1 path is reachable through the existing query-session
enable_strict_castoption, which is propagated to native Parquet materialization. - Compatibility: no function-symbol, storage-format, wire-protocol, or rolling-upgrade contract is changed.
- Parallel paths: safe staged execution, unsafe whole-expression fallback, dense/sparse/dictionary/direct reads, empty selections, later batches, output materialization, and position/equality-delete paths were traced. Their row/cursor alignment is sound; the physical conversion-validation boundary is the remaining semantic gap.
- Conditional checks: the selected-row safety recursion is clear and the unsafe expression fallback is correct, but it checks only VExpr behavior and does not prove that deferred physical decoding is error-free.
- Test coverage: the new all-pass/staged/profile and benchmark-registration tests are useful. Missing coverage is a strict-cast partial-survival invalid-INT96 differential case and benchmark teardown kept outside the timed region.
- Test results: no result file is added or required for these C++ tests, and the asserted rows/counters are internally consistent. I did not run local builds or tests because the governing review task prohibits them. Live CI currently has compile, FE UT, Cloud UT, formatting, style, license, and secret checks passing; BE UT, regression, performance, and the code-review job are still pending.
- Observability: the added residual-stage and alignment counters are appropriate and no extra INFO logging is needed. Existing thread discussion_r3639110107 already covers the remaining shrinking-stage quadratic alignment cost, so it is not duplicated here.
- Transactions, persistence, data writes, and FE/BE variable propagation: not applicable; this is a read-path scheduler/benchmark change and adds no persisted state, transaction boundary, data mutation, or new cross-process variable.
- Performance: the lazy-read direction is sound, but the complex-residual benchmark cannot be trusted for comparison until session/expression teardown is excluded from steady-state timing.
- Other issues: after three review rounds and final changed-file/candidate sweeps, no additional non-duplicate issue remained.
| // space before evaluating it. This is the scanner-side equivalent of expression-triggered | ||
| // lazy columns: a conjunct that rejects the batch prevents later-only columns from decoding. | ||
| for (const auto& stage : schedule.remaining_stages) { | ||
| RETURN_IF_ERROR(materialize_predicate_positions(stage.required_positions)); |
There was a problem hiding this comment.
[P1] Preserve decode failures for rows skipped by later stages
This safety check covers the VExpr tree, but the later column is also subject to physical-to-logical conversion while it is materialized. After an earlier AND child rejects a row, materialize_predicate_positions() calls the native reader with that row filtered out; decode_selected_values() handles FILTERED_CONTENT with skip_values(), and the sparse path runs the SerDe only for selected ranges. For example, with enable_strict_cast=true, an out-of-range INT96 value in a row rejected by stage 1 used to make eager materialization return DataQualityError, while the staged path skips that payload and can return rows successfully. This is below the format::Cast safeguard from the existing thread. Please either limit staged materialization to file-column conversions that are total for every physical value, or validate filtered payloads equivalently to eager decoding, and cover a strict-cast partial-survival invalid-INT96 case.
| } | ||
| struct ReaderSession { | ||
| ~ReaderSession() { |
There was a problem hiding this comment.
[P2] Destroy the expression session while timing is paused
run_reader() resumes timing immediately after reader->close(), but the local session is destroyed only at the end of the loop body. For this new case, that destructor calls context->close() in the measured region. The reader still retains the request that owns the same prepared context, and VExprContext::~VExprContext() closes it again because explicit close() does not clear _opened; request/reader destruction is timed as well. That makes the complex-residual number include duplicate expression teardown even though the benchmark contract excludes setup/close work. Please reset/destroy the session while timing is paused and use a single close ownership path.
hello-stephen
commented
Jul 23, 2026
ClickBench: Total hot run time: 25.06 s |
hello-stephen
commented
Jul 23, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 23, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
commented
Jul 24, 2026
skip buildall |
Uh oh!
There was an error while loading. Please reload this page.
PR approved by at least one committer and no changes requested. |
…pache#65965) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65921 Problem Summary: The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound `AND`) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns. This PR follows expression-triggered lazy materialization: - preserve conjunct order and split safe compound `AND` residuals into ordered expression stages; - record each stage's slot dependencies and materialize only the next reachable columns; - read later columns with the surviving selection, or skip them entirely when an earlier stage rejects the batch; - prefetch only the first reachable predicate stage; - retain the original eager path for stateful/error-sensitive expressions that are unsafe on selected rows; - add unit coverage and a Release microbenchmark scenario based on the Parquet benchmark framework from apache#65921. Correctness/counter validation (6 input rows): - an earlier two-column `AND` child filters all rows: `ReaderReadRows=12`, `ReaderSkipRows=6` (the third column is never decoded); - when 3 rows survive the first residual: the later column reads only those 3 rows (`ReaderReadRows=15`, `ReaderSelectRows=3`, `ReaderSkipRows=3`). Release microbenchmark: ```text CPU: Intel Xeon Platinum 8457C, pinned to CPU 8 Build: RELEASE Filter: ^ParquetReader/complex_residual_scan/plain/null_10/alternating/sel_10/ Protocol: 3 warmups, then A-B-B-A; 5 repetitions per group; min_time=1s Pair 1 median CPU: before 1,377,041 ns; after 1,318,811 ns (-4.23%) Pair 2 median CPU: before 2,140,937 ns; after 2,124,862 ns (-0.75%) Paired geometric normalization: -2.50% CPU time Selected rows: 1,460 in every run ``` The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [x] Manual test (Release microbenchmark above) - [ ] No need to test or manual test. - Behavior changed: - [x] No. The SQL result is unchanged; only predicate-column materialization timing changes. - [ ] 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
…pache#65965) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65921 Problem Summary: The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound `AND`) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns. This PR follows expression-triggered lazy materialization: - preserve conjunct order and split safe compound `AND` residuals into ordered expression stages; - record each stage's slot dependencies and materialize only the next reachable columns; - read later columns with the surviving selection, or skip them entirely when an earlier stage rejects the batch; - prefetch only the first reachable predicate stage; - retain the original eager path for stateful/error-sensitive expressions that are unsafe on selected rows; - add unit coverage and a Release microbenchmark scenario based on the Parquet benchmark framework from apache#65921. Correctness/counter validation (6 input rows): - an earlier two-column `AND` child filters all rows: `ReaderReadRows=12`, `ReaderSkipRows=6` (the third column is never decoded); - when 3 rows survive the first residual: the later column reads only those 3 rows (`ReaderReadRows=15`, `ReaderSelectRows=3`, `ReaderSkipRows=3`). Release microbenchmark: ```text CPU: Intel Xeon Platinum 8457C, pinned to CPU 8 Build: RELEASE Filter: ^ParquetReader/complex_residual_scan/plain/null_10/alternating/sel_10/ Protocol: 3 warmups, then A-B-B-A; 5 repetitions per group; min_time=1s Pair 1 median CPU: before 1,377,041 ns; after 1,318,811 ns (-4.23%) Pair 2 median CPU: before 2,140,937 ns; after 2,124,862 ns (-0.75%) Paired geometric normalization: -2.50% CPU time Selected rows: 1,460 in every run ``` The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [x] Manual test (Release microbenchmark above) - [ ] No need to test or manual test. - Behavior changed: - [x] No. The SQL result is unchanged; only predicate-column materialization timing changes. - [ ] 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
…pache#65965) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65921 Problem Summary: The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound `AND`) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns. This PR follows expression-triggered lazy materialization: - preserve conjunct order and split safe compound `AND` residuals into ordered expression stages; - record each stage's slot dependencies and materialize only the next reachable columns; - read later columns with the surviving selection, or skip them entirely when an earlier stage rejects the batch; - prefetch only the first reachable predicate stage; - retain the original eager path for stateful/error-sensitive expressions that are unsafe on selected rows; - add unit coverage and a Release microbenchmark scenario based on the Parquet benchmark framework from apache#65921. Correctness/counter validation (6 input rows): - an earlier two-column `AND` child filters all rows: `ReaderReadRows=12`, `ReaderSkipRows=6` (the third column is never decoded); - when 3 rows survive the first residual: the later column reads only those 3 rows (`ReaderReadRows=15`, `ReaderSelectRows=3`, `ReaderSkipRows=3`). Release microbenchmark: ```text CPU: Intel Xeon Platinum 8457C, pinned to CPU 8 Build: RELEASE Filter: ^ParquetReader/complex_residual_scan/plain/null_10/alternating/sel_10/ Protocol: 3 warmups, then A-B-B-A; 5 repetitions per group; min_time=1s Pair 1 median CPU: before 1,377,041 ns; after 1,318,811 ns (-4.23%) Pair 2 median CPU: before 2,140,937 ns; after 2,124,862 ns (-0.75%) Paired geometric normalization: -2.50% CPU time Selected rows: 1,460 in every run ``` The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [x] Manual test (Release microbenchmark above) - [ ] No need to test or manual test. - Behavior changed: - [x] No. The SQL result is unchanged; only predicate-column materialization timing changes. - [ ] 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
…pache#65965) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65921 Problem Summary: The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound `AND`) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns. This PR follows expression-triggered lazy materialization: - preserve conjunct order and split safe compound `AND` residuals into ordered expression stages; - record each stage's slot dependencies and materialize only the next reachable columns; - read later columns with the surviving selection, or skip them entirely when an earlier stage rejects the batch; - prefetch only the first reachable predicate stage; - retain the original eager path for stateful/error-sensitive expressions that are unsafe on selected rows; - add unit coverage and a Release microbenchmark scenario based on the Parquet benchmark framework from apache#65921. Correctness/counter validation (6 input rows): - an earlier two-column `AND` child filters all rows: `ReaderReadRows=12`, `ReaderSkipRows=6` (the third column is never decoded); - when 3 rows survive the first residual: the later column reads only those 3 rows (`ReaderReadRows=15`, `ReaderSelectRows=3`, `ReaderSkipRows=3`). Release microbenchmark: ```text CPU: Intel Xeon Platinum 8457C, pinned to CPU 8 Build: RELEASE Filter: ^ParquetReader/complex_residual_scan/plain/null_10/alternating/sel_10/ Protocol: 3 warmups, then A-B-B-A; 5 repetitions per group; min_time=1s Pair 1 median CPU: before 1,377,041 ns; after 1,318,811 ns (-4.23%) Pair 2 median CPU: before 2,140,937 ns; after 2,124,862 ns (-0.75%) Paired geometric normalization: -2.50% CPU time Selected rows: 1,460 in every run ``` The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [x] Manual test (Release microbenchmark above) - [ ] No need to test or manual test. - Behavior changed: - [x] No. The SQL result is unchanged; only predicate-column materialization timing changes. - [ ] 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
## Proposed changes Backport the requested changes to `branch-4.1` in master merge order, skipping changes already present in this PR: 1. #62438 2. #65329 (merged prerequisite for the nested-schema cases) 3. #65960 4. #65965 5. #65972 6. #65998 7. #66002 8. #65992 9. #66021 10. #66036 11. #66008 12. #66073 13. #66056 (explicitly requested; current open-PR head, appended after the merged sequence) The branch-specific compatibility commits preserve the selected master behavior on `branch-4.1`, including master wire IDs for file formats and the merged Paimon test helper prerequisite. No regression expected output, test assertion, or test input was changed to make validation pass. ## Validation - Full BE ASAN build passed. - Full FE build passed. - Targeted BE ASAN unit tests: 332 tests from 13 suites passed. - Targeted FE Iceberg unit tests: 41 passed, 0 failed. - Iceberg write regression: 20 suites, 0 failed, 0 fatal. - `PaimonScanNodeTest`: all 16 test bodies completed with 0 assertion failures; the class reports one Mockito teardown error because #66008 left four now-unused stubs on master. The still-open #65867 contains the upstream test-only cleanup commit. This PR intentionally does not alter those test cases. - Final rebase against the latest `branch-4.1` completed; the branch was already up to date. - Working-tree, formatting, and sensitive-information audits completed. Existing EOF blank lines in picked regression output files are preserved. --------- Co-authored-by: daidai <changyuwei@selectdb.com> Co-authored-by: Mingyu Chen (Rayner) <yunyou@selectdb.com>
…pache#65965) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65921 Problem Summary: The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound `AND`) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns. This PR follows expression-triggered lazy materialization: - preserve conjunct order and split safe compound `AND` residuals into ordered expression stages; - record each stage's slot dependencies and materialize only the next reachable columns; - read later columns with the surviving selection, or skip them entirely when an earlier stage rejects the batch; - prefetch only the first reachable predicate stage; - retain the original eager path for stateful/error-sensitive expressions that are unsafe on selected rows; - add unit coverage and a Release microbenchmark scenario based on the Parquet benchmark framework from apache#65921. Correctness/counter validation (6 input rows): - an earlier two-column `AND` child filters all rows: `ReaderReadRows=12`, `ReaderSkipRows=6` (the third column is never decoded); - when 3 rows survive the first residual: the later column reads only those 3 rows (`ReaderReadRows=15`, `ReaderSelectRows=3`, `ReaderSkipRows=3`). Release microbenchmark: ```text CPU: Intel Xeon Platinum 8457C, pinned to CPU 8 Build: RELEASE Filter: ^ParquetReader/complex_residual_scan/plain/null_10/alternating/sel_10/ Protocol: 3 warmups, then A-B-B-A; 5 repetitions per group; min_time=1s Pair 1 median CPU: before 1,377,041 ns; after 1,318,811 ns (-4.23%) Pair 2 median CPU: before 2,140,937 ns; after 2,124,862 ns (-0.75%) Paired geometric normalization: -2.50% CPU time Selected rows: 1,460 in every run ``` The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [x] Manual test (Release microbenchmark above) - [ ] No need to test or manual test. - Behavior changed: - [x] No. The SQL result is unchanged; only predicate-column materialization timing changes. - [ ] 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
What problem does this PR solve?
Issue Number: N/A
Related PR: #65921
Problem Summary:
The V2 Parquet scanner already evaluates single-column predicates round by round, but a multi-column residual (including the children of a compound
AND) still caused every predicate column to be materialized before expression evaluation. Expression short circuiting therefore happened after Parquet decode/IO and could not avoid later-only columns.This PR follows expression-triggered lazy materialization:
ANDresiduals into ordered expression stages;Correctness/counter validation (6 input rows):
ANDchild filters all rows:ReaderReadRows=12,ReaderSkipRows=6(the third column is never decoded);ReaderReadRows=15,ReaderSelectRows=3,ReaderSkipRows=3).Release microbenchmark:
The host had high concurrent load and CPU scaling enabled, so the paired groups and run conditions are reported explicitly rather than relying on wall time. Both A/B pairs improve in the same direction.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)