[improvement](parquet) Lazily materialize complex residual columns#65965
Open
Gabriel39 wants to merge 3 commits into
Open
[improvement](parquet) Lazily materialize complex residual columns#65965Gabriel39 wants to merge 3 commits into
Gabriel39 wants to merge 3 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
marked this pull request as ready for review
July 23, 2026 11:39
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 29295 ms |
Contributor
TPC-DS: Total hot run time: 177005 ms |
Contributor
ClickBench: Total hot run time: 24.91 s |
Gabriel39
force-pushed
the
agent/parquet-complex-residual-lazy-materialization
branch
from
July 23, 2026 12:14
bbbdeb1 to
913663e
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 29235 ms |
Contributor
TPC-DS: Total hot run time: 178100 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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.
Gabriel39
force-pushed
the
agent/parquet-complex-residual-lazy-materialization
branch
from
July 23, 2026 15:56
913663e to
8eb8cdb
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29391 ms |
Contributor
TPC-DS: Total hot run time: 177149 ms |
Contributor
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)); |
Contributor
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() { |
Contributor
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.
Contributor
ClickBench: Total hot run time: 25.06 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
2 participants
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.
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)