morningman opened a new pull request, #69579:
URL: https://github.com/apache/doris/pull/69579
### 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::push` timed itself with the same
`ExecTime` counter that the operator framework already uses around every
`push()`, so the operator's `ExecTime` counted each push twice, including the
synchronous row-fetch RPC wait. `push()` no longer opens its own timer.
Original author: @Gabriel39
**Background.** `MaterializationOperator` is 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 inside
`push()`. It is a `StatefulOperatorX`, and `StatefulOperatorX::get_block_impl`
already wraps every `push()` and `pull()` in
`SCOPED_TIMER(local_state.exec_time_counter())`.
**The problem, and what it cost.** `MaterializationOperator::push` opened a
second `SCOPED_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 inside
`push()`, the profile reported a materialization `ExecTime` up 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 the
`ExecTime` sample 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.**
| | `ExecTime` of one push |
|---|---|
| Before | counted twice (framework timer + the nested one in `push()`) |
| After | counted once, by the framework |
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
-
`MaterializationOperatorTimingTest.PushDoesNotDuplicateFrameworkExecTimer`
(from #68690): 100 direct `push()` calls, which bypass the framework, leave
`ExecTime` unchanged. With the nested timer back, it fails.
`MaterializationSharedStateTest` still passes.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
