Repository navigation
[fix](be) Count the materialization push time once - #69579
Merged
morningman merged 1 commit intoOct 26, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#68690 Problem Summary: MaterializationOperator is a StatefulOperatorX, whose get_block_impl already wraps push() in SCOPED_TIMER(exec_time_counter()). MaterializationOperator::push opened a second SCOPED_TIMER on the same counter, so every push, including the synchronous row-fetch RPC wait of the second phase of a TopN lazy materialization, was added to ExecTime twice and the operator's profile overstated its execution time. This is the timing fix of branch-4.1 apache#68690, which is otherwise about Lance; the Lance parts follow with the Lance work. push() no longer opens its own timer, and the framework counts it once. ### Release note None ### Check List (For Author) - Test: Unit Test - materialization_shared_state_test.cpp: 100 direct push() calls leave ExecTime unchanged; the framework owns the sample - Behavior changed: No (profile counter only) - Does this need documentation: No Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of head 7ff57c15e00a6fa969145c0c75601235b9ecdaed found no actionable new defects in the complete two-file diff.
StatefulOperatorX::get_block_impl scopes ExecTime around push() and pull() independently (be/src/exec/operator/operator.cpp:743-772). Removing the nested timer leaves the synchronous row-fetch wait and early error returns accounted once, with child execution outside these scopes. MaxRpcTime and MergeResponseTime remain intact.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and supporting test | The change corrects duplicate materialization timing. The new test checks that 100 direct empty, non-EOS pushes add no second ExecTime sample; its assertions match that path. |
| Scope and clarity | One redundant production timer is removed, with an accurate explanatory comment and a focused test. |
| Concurrency and locks | No new thread, lock scope, lock order, atomic, or dependency change. Framework timers use task-local state; existing RPC callback/countdown coordination is preserved. |
| Lifecycle and static initialization | Tuple lookup, factory creation, local-state registration and ownership were traced. Default destructors and non-owning profile child pointers make this empty fixture's teardown valid. No new static initialization or ownership cycle. |
| Configuration | No additions or dynamic configuration changes. |
| Compatibility and rolling upgrades | No function signatures, storage formats, wire contracts or new libc dependencies change. |
| Thrift/protobuf definitions | No gensrc change; cross-branch field-id comparison is not applicable. |
| Parallel paths | Production materialization uses the inherited timed dispatcher; no bypassing production push caller was found. Pull already has no duplicate local timer. |
| Conditions and comments | No production condition changes; the new comment correctly explains timer ownership and duplicate RPC-wait accounting. |
| Test coverage | Existing BE-UT discovery and private-access conventions cover the added test. Its initialized-RPC flag and empty input deliberately avoid expressions and network calls. Nonempty/EOS/error wrapper paths were checked statically. |
| Changed expected results | No golden files changed; unchanged-counter equality is appropriate for the exercised direct-call path. |
| Observability | ExecTime attribution is corrected; dedicated RPC/merge metrics remain. No additional logging or counters are needed. |
| Transactions, persistence and failover | No affected transaction, EditLog, version, bitmap or persistence logic. |
| Data writes and atomicity | No write, crash-recovery or atomicity behavior changes. |
| FE-BE variables | No transmitted variables or send paths change. |
| Performance and memory | Removes redundant stopwatch reads/counter updates; adds no production allocation. Test objects have bounded RAII/ObjectPool ownership. |
| Other issues and core invariants | Data visibility, version/delete-bitmap consistency, Status propagation, reservations and nullable/const handling remain unchanged. No additional substantiated issue found. |
No additional user focus was supplied. There are no existing inline findings to carry forward, and no unresolved code candidates or uncovered changed files remain after the complete review and separate fixture-risk review.
Validation limits: static review only, as required by this runner. No build, test execution, runtime timing measurement or ELF audit was performed. The new test does not establish distributed RPC timing or positive framework timing empirically; the unchanged framework scopes were inspected in code.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28309 ms |
Contributor
TPC-DS: Total hot run time: 150671 ms |
Contributor
ClickBench: Total hot run time: 24.46 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3 of 12 tasks
morningman
added a commit
to morningman/doris
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65730 apache#66608 apache#66805 apache#66841 apache#66999 apache#67005 apache#67011 apache#67110 apache#67268 apache#67289 apache#67325 apache#67650 apache#67921 apache#68028 apache#68335 apache#68375 apache#68615 apache#68690 Problem Summary: Port the Lance BE read path of branch-4.1/4.2 (as of branch-4.2 cabcf45) to master. Master links liblance_c but has no Lance reader: file_scan_operator keeps FORMAT_LANCE off FileScannerV2 and V1 has no Lance path, so every FORMAT_LANCE scan fails today. This change is BE only. The FE connector plugin and the regression suites follow separately, so nothing on master plans a Lance scan yet and the BE unit tests carry the proof. - Reader: be/src/format_v2/lance/* and format_v2/table/lance_reader.* (plain scans, vector and full-text search, Substrait and runtime filter pushdown, scalar index segments, COUNT(*) from metadata, LIMIT, profiles), copied from branch-4.2 except one local renamed for -Wshadow. - Hooks: FileFormat::LANCE and its exhaustive switches, FileScannerV2 dispatch, and fetch_table_schema for FORMAT_LANCE. Lance queries use FileScannerV2 whatever enable_file_scanner_v2 says, as ADBC and fluss do: there is no V1 Lance reader. The second phase of TopN lazy materialization reads Lance rows through dataset take_rows. - Row locations: GlobalRowLoacationV2 gets a version 1 layout with a 64-bit Lance row id (the version 0 layout is unchanged), PRequestBlockDesc.row_id becomes uint64 (varint, wire compatible), and the materialization operator and RowIdStorageReader decode and validate by version. - Thrift: the external search request body, TLanceFileDesc 4-6 and TExternalSearchRequest 2-4 with the branch-4.1/4.2 ids. - Core: PTypeDesc keeps NULL_TYPE for a NULL literal type, and the nullable serde reads an Arrow Null array as NULLs. - Nine lance_* BE configs, a BE-wide Lance session with an optional Foyer data cache and 10 lance_session_* metrics; macOS links IOKit. - Tests: the Lance BE unit tests and their fixtures, including the docker fixture all_types.lance that one of them reads. Under master's -ffile-prefix-map the session test locates its fixture from the repository root, and the reader tests shrink the data cache to 64 MiB under the UT temp dir: the default 100 GiB disk tier is 51200 block files that stay open for the rest of the test process. Not included: the MaterializationOperator::push timer fix (apache#69579), the Lance index metadata scanner, the FE side, regression suites, and the 4.x changes after cabcf45 (apache#68707, apache#68813, apache#68815). ### Release note Add nine lance_* BE configs and lance_session_* BE metrics. They take effect once the FE can plan Lance scans. ### Check List (For Author) - Test: Unit Test - BE UT: the Lance reader, nested Null, runtime filter and session suites, plus the row-location protocol and FileScannerV2 cases - Behavior changed: Yes. PRequestBlockDesc.row_id is uint64, and the second phase of TopN lazy materialization rejects row locations of an unknown version or size. - Does this need documentation: No (documented with the FE connector) Co-authored-by: zhangstar333 <zhangsida@selectdb.com> Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-authored-by: zhaobo wang <wangzhaobo957@gmail.com> Co-authored-by: jay <qingfengyouchen@163.com> Co-authored-by: FANNG <fanng@apache.org> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Gabriel39
approved these changes
Oct 26, 2026
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.
What problem does this PR solve?
Issue Number: None
Related PR: #68690
Problem Summary:
In short. This ports the materialization timing fix of branch-4.1 #68690 to master.
MaterializationOperator::pushtimed itself with the sameExecTimecounter that the operator framework already uses around everypush(), so the operator'sExecTimecounted each push twice, including the synchronous row-fetch RPC wait.push()no longer opens its own timer.Original author: @Gabriel39
Background.
MaterializationOperatoris the second phase of TopN lazy materialization: it collects row IDs from its child and fetches the remaining columns from the backends that own the rows, waiting for the fetch RPCs insidepush(). It is aStatefulOperatorX, andStatefulOperatorX::get_block_implalready wraps everypush()andpull()inSCOPED_TIMER(local_state.exec_time_counter()).The problem, and what it cost.
MaterializationOperator::pushopened a secondSCOPED_TIMER(local_state.exec_time_counter()). Both timers add their elapsed time to the same counter when they go out of scope, so the time of every push was added twice. Since the row-fetch RPC wait happens insidepush(), the profile reported a materializationExecTimeup to twice the real time, which points at the wrong operator when a TopN lazy materialization query is slow. Only the profile is affected; results and the fetch itself are not.How this PR fixes it.
push()drops its own timer and leaves theExecTimesample to the framework, with a comment explaining why.pull()never had the nested timer. #68690 also adds Lance row-fetch counters and a Lance query parallelism option; those belong to the Lance work and are not part of this PR.Results.
ExecTimeof one pushpush())Release note
None
Check List (For Author)
MaterializationOperatorTimingTest.PushDoesNotDuplicateFrameworkExecTimer(from [improvement](lance) Expose query parallelism and complete row fetch profiles (branch-4.1) #68690): 100 directpush()calls, which bypass the framework, leaveExecTimeunchanged. With the nested timer back, it fails.MaterializationSharedStateTeststill passes.Check List (For Reviewer who merge this PR)