Doris-Breakwater commented on issue #67040: URL: https://github.com/apache/doris/issues/67040#issuecomment-5379355140
## Breakwater initial analysis **Assessment: confirmed bug, high confidence.** This is an observability defect in the spill-read profile path; the inspected path does not indicate an effect on query results or spill execution itself. ### Verified facts - At commit `5202d06dd8`, `PipelineXSpillLocalState::init()` initializes `SpillReadCounters` on `CustomCounters`, and `SpillReadCounters::init()` registers `profile::SPILL_READ_DESERIALIZE_BLOCK_TIME`, whose value is `SpillReadDeserializeBlockTime`. - `SpillFileReader` instead looks up `SpillReadDerializeBlockTime`. `RuntimeProfile::get_counter()` returns `nullptr` when that name is absent. - `SCOPED_TIMER(_deserialize_timer)` surrounds the decode work in `SpillFileReader::read()`, but `ScopedTimer` deliberately performs no timing or update when its counter is null. The correctly registered production counter therefore remains at its initial value. - The same misspelled name is manually registered in `spill_file_test.cpp` and `spill_repartitioner_test.cpp`, so those fixtures give the reader a non-null counter and mask the production mismatch. The existing `ReadCounters` test checks block, row, and byte counters but not the deserialize timer. - The typo is isolated to these three occurrences in the inspected tree: the reader lookup and the two test fixtures. One semantic nuance: the current timer scope includes both protobuf `ParseFromArray()` and `Block::deserialize()`. Correcting the lookup will measure that existing combined decode scope; narrowing it to only `Block::deserialize()` would be a separate semantics decision. ### Missing information No additional version, logs, profile, or reproduction details are required to confirm the root cause. The issue identifies an exact revision and a deterministic registration/lookup mismatch. The issue currently has no labels, assignee, milestone, or linked PR. ### Recommended next steps 1. Accept this as a valid, narrowly scoped profile-counter bug and let the reporter submit the offered PR. 2. Resolve the reader's counter names through the shared constants (using a fully qualified name such as `doris::profile::SPILL_READ_DESERIALIZE_BLOCK_TIME` where the `profile` function parameter would otherwise shadow the namespace). Prefer converting the other spill reader lookups at the same time to prevent another silent string drift. 3. Correct both test fixtures to register the canonical name, ideally through the same shared counter initialization path used in production. 4. Extend `SpillFileTest.ReadCounters` to assert that the canonical `SpillReadDeserializeBlockTime` counter exists and is updated after an actual spill-file read. This is the regression assertion currently missing. 5. Run the spill file and spill repartitioner BE unit tests and the normal BE formatting checks. A full cluster/profile reproduction is useful as end-to-end confirmation but is not necessary to establish this code-level defect. Breakwater-GitHub-Analysis-Slot: slot_51a4a1e991e2 -- 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]
