Fix read_rows reported as zero when a read step materializes no column from the part - #119400
Conversation
A step that reads no column from disk reported no rows read, so `read_rows` in query_log, query progress, the SelectedRows profile events and the `max_rows_to_read` accounting derived from them all lost those rows, while the query itself returned correct data. The reachable shape is a MergeTree part with a pending lightweight update. Once a patch part exists, the front step of the readers chain is the lightweight delete step, whose column list is just `_row_exists`, which has no file in the part. With `patch_parts_version = 'v1'` in Merge mode the patch appends only virtual columns, so on a Wide part `MergeTreeReaderWide::readRows` returns 0, where the compact reader returns the granularity-derived count. Only the front step of the chain accumulates the counter, so the whole query exported 0: both `DELETE FROM t WHERE id IN (100, 110, 120, 130)` and a plain `SELECT sum(id) FROM t` reported `read_rows = 0`. The row count itself was already normalized here, falling back to the granule sizes taken from the index; the exported counter was not. Normalizing it on the same path, after `adjustLastGranule()`, keeps a partial final granule at its real size and cannot double count, since `startReadingChain` runs once per read and `continueReadingChain` never accumulates. `continueReadingChain` has always normalized its own count the same way, so this only makes the front step of a readers chain consistent with every later step. It also removes an underflow at `MergeTreeReadTask.cpp:507`, which computed `numReadRows() - num_rows` in UInt64 while the counter stayed at zero. Introduced by ClickHouse#114562, which removed the minimum-size carrier column from `injectRequiredColumns` on the premise that a read of no columns still reports the right row count from the index granularity. That premise held for `ReadResult::num_rows` and not for `ReadResult::numReadRows()`; this makes it true for both, without reading a column from disk to learn a row count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Internal second-model reviewThree rounds, seven findings adopted, one declined; the last independent pass returned none.
💡 Also fixed before publishing: the documented contract of |
|
Workflow [PR], commit [6deb19f] Summary: ✅
AI ReviewSummaryThis PR fixes Final Verdict✅ No new findings. LLVM Coverage ReportMeasured on commit 6deb19f.
Changed lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code |
|
cc @Avogar @tiandiwonder, could you review this? On a Wide part with a pending lightweight update the front step of the readers chain requests only |
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units7 translation units recompiled, 11 s compile time in total, 7 of them have a recent master baseline. |
`05175_lwu_delete_read_rows_wide_part` asserts exact `read_rows` values. When a read range is split by primary key, both layers read the granule that holds the split boundary, so its rows are counted twice: the non-adaptive full scan reports 1064 instead of 1000, and the four scans stop agreeing on a single count. That over-read belongs to the injection, not to the change under test. On the binary without this fix, the same scan of a table with no patch part at all reports 1064, and so does the `patch_parts_version = 'v2'` arm, which reads a physical column and is already correct. So the setting is pinned instead of the expected values relaxed. A `SET` rather than a `Random settings limits` comment: the limit only clamps the values `tests/clickhouse-test` generates itself, while `ci/jobs/scripts/stress/stress.py` passes this setting as a `--client-option` (0.05, on every fifth stress thread) which wins over the generated ones, and the stress runner selects s3 storage for a third of its runs. A `SET` inside the test wins over both, as in `00926_adaptive_index_granularity_pk.sql` and `00945_bloom_filter_index.sql`. Reported by CI on this PR in `Stateless tests (amd_tsan, s3 storage, parallel, selected tests)`, where the tsan build keeps settings randomization that the debug s3 jobs do not. Reproduced against a server on s3: 7 of 10 runs at the values the runner generates and 8 of 10 with 0.9 forced on the command line, against 0 of 50 randomized runs and 0 of 20 forced runs with the `SET` in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Include the upstream row-count repairs needed by the completed CI runs. Resolve the connection conflict by retaining profile-setting preparation and using the shared network compression codec selector from master. Preserve the trace subscription lifecycle, distributed forwarding and protocol negotiation while retaining the upstream shutdown, leaf-timeout and preauthentication changes. Related: ClickHouse#119429 Related: ClickHouse#119400 Related: ClickHouse#117830 (comment) Co-Authored-By: Codex CLI <noreply@openai.com>
Related: #114562
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Not for changelog: the defect was introduced on master by #114562 and has never shipped in a release, so no released version is affected.
Description
Any read of a MergeTree part with a pending lightweight update reports
read_rows = 0when the part is Wide andpatch_parts_version = 'v1'. The rows are read and the results are correct; the accounting is lost:system.query_log.read_rows, query progress, theSelectedRowsprofile events andread_rows-derived limits such asmax_rows_to_read. Measured on592da49d0a3b6, plainMergeTree, no randomization:DELETE FROM t WHERE id IN (100, 110, 120, 130)SELECT sum(id) FROM tSELECT count() FROM t WHERE id = 500Origin: #114562. It removed the minimum-size carrier column from
injectRequiredColumns, on the premise that a read of no columns still reports the right row count from the index granularity. That premise holds forReadResult::num_rowsand fails forReadResult::numReadRows(), the exported counter.03100_lwu_deletes_4_indexwent red on master 23 minutes after that merge, in 5 of the 13 master runs of592da49d0a3b6and across four build flavours, against 0 of 12 on its predecessorbf549ba1b4fe0and 0 of 7120 over the preceding 20 days.The fix accounts for the granule-derived rows in
MergeTreeRangeReader::startReadingChain, afteradjustLastGranule(), wherenum_rowsis already normalized.continueReadingChainhas always normalized its own count the same way, so this only makes the front step of a readers chain consistent with every later step. Only the front step accumulates the exported counter, so it cannot double count.The new test fails on
592da49d0a3b6and passes with the fix, and runs clean 50/50 with and without settings randomization. In a patch mode x part type x granularity matrix measured on both binaries, the fix repairs every zero-reporting cell, including non-adaptive Wide and a partial final granule, and leaves the rest byte-identical.Master CI report: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?REF=master&sha=592da49d0a3b6d3408bf646e71d73aec8ac246ee&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_debug%2C%20parallel%29
Workflow [PR]
Sync PR [sync-upstream/pr/119400]
Version info
26.9.1.1247(included in26.9and later)