Uh oh!
There was an error while loading. Please reload this page.
[fix](spill) Record the spill read deserialize timer - #67041
Conversation
SpillFileReader looked the deserialize timer up as
"SpillReadDerializeBlockTime", while SpillReadCounters::init registers it under
profile::SPILL_READ_DESERIALIZE_BLOCK_TIME ("SpillReadDeserializeBlockTime").
get_counter() therefore returned null and ScopedTimer skipped it, so
SpillReadDeserializeBlockTime stayed at 0 in every query profile.
Look every spill read counter up through the shared profile:: name constants
instead of string literals, and DCHECK that each one resolves, so a future
rename fails loudly instead of silently dropping a counter.
The unit tests registered the same misspelling by hand, which is why the gap
went unnoticed: the lookup matched in tests while returning null in a real
query. They now reuse SpillWriteCounters::init / SpillReadCounters::init so the
registered names and units cannot drift from production again. That also
corrects spillable_operator_test_helper, which registered the two read timers
as TUnit::UNIT and would trip the type DCHECK inside ScopedTimer once the timer
resolves.hello-stephen
commented
Aug 22, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
zhuxiangyi
commented
Aug 22, 2026
run buildall |
hello-stephen
commented
Aug 22, 2026
TPC-H: Total hot run time: 17417 ms |
hello-stephen
commented
Aug 22, 2026
TPC-DS: Total hot run time: 84541 ms |
hello-stephen
commented
Aug 22, 2026
ClickBench: Total hot run time: 14.64 s |
zhuxiangyi
commented
Aug 22, 2026
run beut |
hello-stephen
commented
Aug 22, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
commented
Aug 24, 2026
/review |
There was a problem hiding this comment.
Review opinion: looks good; I found no blocking correctness issue and no inline finding to submit.
Critical checkpoint conclusions:
- Goal and proof: The reader now resolves
SpillReadDeserializeBlockTimethrough the same canonical constant used bySpillReadCounters::init, so the existing profile counter is updated instead of a misspelled lookup returning null.ReadDeserializeTimerIsRecordedstarts from a fresh zero-valued canonical counter, performs a real spill read, requires it to become positive, and confirms the old misspelled name is absent; it would fail with the old lookup. - Scope and clarity: The production change is confined to counter lookup. The three fixture changes reuse the production counter initializers, preserve all reader/writer-required extras, and correct the former timer/byte unit mismatches without changing runtime data behavior.
- Concurrency: No execution scheduling or shared-state protocol changes. RuntimeProfile map access remains locked, counter values remain atomic, and every reader is constructed only after its owning local state has initialized the profile counters.
- Lifecycle: No ownership or release behavior changes. The stack-local
SpillWriteCounters/SpillReadCountersholders in tests only retain aliases; RuntimeProfile owns the counters for the fixture lifetime. - Configuration and compatibility: No configuration, FE-BE field, symbol, storage-format, transaction, persistence, or rolling-upgrade change. The externally visible counter name is unchanged.
- Parallel paths and conditions: All production SpillFileReader construction paths were traced, including spill sort, aggregation, hash join, repartitioning, multi-cast, and Iceberg sorted spill; each receives the canonical read-counter set with matching TIME_NS/UNIT/BYTES types before construction. The new DCHECKs express that registration invariant.
- Tests and results: The added BE unit test directly covers the regression, and the shared fixture setup prevents future test/production name drift. The exact PR head currently has passing COMPILE and BE UT checks; no local build was run in this review-only runner.
- Observability: This directly restores the existing deserialize-time signal and does not add or rename a metric. Existing sibling spill counters and identifiers remain unchanged.
- Performance and memory: Counter resolution remains constructor-only; the read hot path and allocation behavior are unchanged.
- Additional/user focus: No additional user focus was provided. The full changed-file sweep and round-1 full/risk reviews found no further valuable issue.
Review completeness: complete after one round; both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS, and the main-agent reconciliation left no unresolved candidate.
Uh oh!
There was an error while loading. Please reload this page.
… (#67059) ### What problem does this PR solve? Issue Number: close#67040 Related PR: #67041 Problem Summary: Cherry-picked from #67041. The automatic pick failed with conflicts in the three test files, so this is a manual backport. `SpillReadDeserializeBlockTime` is always `0` in query profiles on branch-4.1 — the time spent deserializing spilled blocks is never recorded. `PipelineXSpillLocalState` registers the counter as `"SpillReadDeserializeBlockTime"` (`be/src/exec/operator/operator.h`), but `SpillFileReader` looked it up with a literal that is missing an `s`: ```cpp // be/src/exec/spill/spill_file_reader.cpp _deserialize_timer = custom_profile->get_counter("SpillReadDerializeBlockTime"); // ^^^ Derialize ``` `get_counter()` returns `nullptr` for the unknown name and `ScopedTimer` returns early on a null counter, so `SCOPED_TIMER(_deserialize_timer)` in `SpillFileReader::read()` silently measures nothing. **Difference from the master PR:** #67041 resolves every spill read counter through the shared `profile::` name constants, but `be/src/runtime/runtime_profile_counter_names.h` does not exist on branch-4.1. This backport therefore keeps the minimal spelling fix rather than porting the constant refactor — that is also what caused the automatic cherry-pick conflict. Changes: 1. `SpillFileReader` looks the counter up under the name the operator actually registers. 2. `spill_file_test.cpp` and `spill_repartitioner_test.cpp` registered the same misspelling by hand, which is why the gap went unnoticed: the reader's lookup matched in tests while returning null in a real query. They now register the canonical name. 3. `spillable_operator_test_helper` registered `SpillReadFileTime` and `SpillReadDeserializeBlockTime` as `TUnit::UNIT`. That stayed harmless only while the deserialize lookup resolved to null; once it resolves, `SCOPED_TIMER`'s `DCHECK_EQ(counter->type(), TUnit::TIME_NS)` would fire, so both are registered as timers. ### Release note Fix `SpillReadDeserializeBlockTime` always being 0 in query profiles. ### Check List (For Author) - Test - [x] Unit Test Ported `SpillFileTest.ReadDeserializeTimerIsRecorded` from #67041. It fails before this change: with the test registering the canonical name, the reader's misspelled lookup yields a null counter and the timer stays at 0 after a real read. The test also asserts the misspelled name is absent, so it cannot be reintroduced. - Behavior changed: - [x] No. Profile-only fix. The counter already existed and was already reported; it was simply never updated. No counter is added, removed, or renamed. - Does this need documentation? - [x] No.
What problem does this PR solve?
Issue Number: close#67040
Problem Summary:
SpillReadDeserializeBlockTimeis always0in query profiles — the time spent deserializing spilled blocks has never been recorded.The counter is registered by
SpillReadCounters::initunder the shared constantprofile::SPILL_READ_DESERIALIZE_BLOCK_TIME, whose value is"SpillReadDeserializeBlockTime":But
SpillFileReaderlooked it up with a hand-written literal that is missing ans:get_counter()returnsnullptrfor the unknown name, andScopedTimerreturns early on a null counter, soSCOPED_TIMER(_deserialize_timer)inSpillFileReader::read()silently measures nothing.Why existing tests did not catch it:
spill_file_test.cppandspill_repartitioner_test.cppregistered the counters by hand and copied the same misspelling, so the reader's lookup resolved in tests while returning null in a real query.Changes:
SpillFileReadernow resolves every read counter through theprofile::name constants instead of string literals, andDCHECKs that each one resolves, so a future rename fails loudly instead of silently dropping a counter.SpillWriteCounters::init/SpillReadCounters::initrather than re-listing the names, so the registered names and units cannot drift from production again.spillable_operator_test_helperregisteredSpillReadFileTimeandSpillReadDeserializeBlockTimeasTUnit::UNITinstead of timers. That was harmless while the timer resolved to null, but trips theDCHECK_EQ(counter->type(), TUnit::TIME_NS)insideScopedTimeronce it resolves, so it is corrected by the same reuse.Release note
Fix
SpillReadDeserializeBlockTimealways being 0 in query profiles.Check List (For Author)
Test
Unit Test
Added
SpillFileTest.ReadDeserializeTimerIsRecorded, which fails before thischange: with the test registering the canonical name, the reader's misspelled
lookup yields a null counter and the timer stays at 0 after a real read. The
test also asserts the misspelled name is absent, so it cannot be reintroduced.
Behavior changed:
No.
Profile-only fix. The counter already existed and was already reported; it was
simply never updated. No counter is added, removed, or renamed.
Does this need documentation?