Skip to content

Fix read_rows reported as zero when a read step materializes no column from the part - #119400

Merged
alexey-milovidov merged 2 commits into
ClickHouse:masterfrom
groeneai:fix-read-rows-zero-no-physical-column
Sep 12, 2026
Merged

Fix read_rows reported as zero when a read step materializes no column from the part#119400
alexey-milovidov merged 2 commits into
ClickHouse:masterfrom
groeneai:fix-read-rows-zero-no-physical-column

Conversation

@groeneai

@groeneai groeneai commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Related: #114562

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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 = 0 when the part is Wide and patch_parts_version = 'v1'. The rows are read and the results are correct; the accounting is lost: system.query_log.read_rows, query progress, the SelectedRows profile events and read_rows-derived limits such as max_rows_to_read. Measured on 592da49d0a3b6, plain MergeTree, no randomization:

query master this PR
DELETE FROM t WHERE id IN (100, 110, 120, 130) 0 8
SELECT sum(id) FROM t 0 1000
SELECT count() FROM t WHERE id = 500 0 2

Origin: #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 for ReadResult::num_rows and fails for ReadResult::numReadRows(), the exported counter. 03100_lwu_deletes_4_index went red on master 23 minutes after that merge, in 5 of the 13 master runs of 592da49d0a3b6 and across four build flavours, against 0 of 12 on its predecessor bf549ba1b4fe0 and 0 of 7120 over the preceding 20 days.

The fix accounts for the granule-derived rows in MergeTreeRangeReader::startReadingChain, after adjustLastGranule(), where num_rows is already normalized. 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. Only the front step accumulates the exported counter, so it cannot double count.

The new test fails on 592da49d0a3b6 and 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

  • Merged into: 26.9.1.1247 (included in 26.9 and later)

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>
@groeneai groeneai added can be tested Allows running workflows for external contributors groeneai-origin-ci-master PR origin: master/nightly CI monitoring finding labels Sep 11, 2026
@groeneai

Copy link
Copy Markdown
Collaborator Author
Internal second-model review

Three rounds, seven findings adopted, one declined; the last independent pass returned none.

⚠️ Adopted on its second raise: a committed arm for a non-adaptive Wide part
(index_granularity_bytes = 0, granularity 64 over 1000 rows) whose final granule is partial. That
combination is worth an assertion, because a constant-granularity part's last mark is padded to a
full granule and corrected only afterwards by fixFromRowsCount, while on this path
adjustLastGranule() returns early, so nothing physical trims the tail. Its scan asserts exactly
1000 rows; a padded last mark would report 1024.

⚠️ Declined: an oracle proving that no on-disk column was read, and an exact absolute read_rows for
the other scans instead of agreement between the arms.

  • Such an oracle would pin the implementation rather than the behaviour, and would redden on a
    legitimate alternative fix such as re-introducing a carrier column. read_bytes cannot serve as
    it: the main reader still reads id, so it is non-zero on both binaries.
  • The scans assert that the arms agree and that the count is non-zero, and the
    patch_parts_version = 'v2' arm reads the physical sorting key, so agreement is equality against
    ground truth rather than a slack threshold; both clauses flip on master. The DELETE counts are
    pinned exactly, because 03100_lwu_deletes_4_index shows those values are stable across flavours,
    and the one full-scan absolute that is pinned is the non-adaptive arm, where the expected number
    comes from the INSERT.

💡 Also fixed before publishing: the documented contract of ReadResult::num_read_rows still said it
may be zero when no read column is present in the part, which is the case this PR normalizes; the
changelog category; a test comment that stated a mechanism that was not the mechanism; and the
bisection figures above, re-derived from the CI database this round rather than carried over,
counting master runs of the test that were not skipped.

@clickhouse-gh clickhouse-gh Bot closed this Sep 11, 2026
@clickhouse-gh clickhouse-gh Bot reopened this Sep 11, 2026
@clickhouse-gh

clickhouse-gh Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [6deb19f]

Summary:


AI Review

Summary

This PR fixes MergeTreeRangeReader so the first step of a readers chain accounts rows from the granule layout even when it materializes no on-disk column, which restores exported read_rows accounting for the Wide patch_parts_version = 'v1' lightweight-update path. I traced the updated num_read_rows / num_rows invariants through startReadingChain, later reader stages, progress reporting, and the new stateless coverage, and I did not find a remaining correctness or test gap in the current patch.

Final Verdict

✅ No new findings.

LLVM Coverage Report

Measured on commit 6deb19f.

Metric Baseline Current Δ
Lines 89.00% 88.90% -0.10%
Functions 91.80% 91.80% +0.00%
Branches 81.30% 81.30% +0.00%

Changed lines: Changed C/C++ lines covered: 6/6 (100.00%) · Uncovered code

Full report · Diff report

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Sep 11, 2026
@groeneai

Copy link
Copy Markdown
Collaborator Author

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 _row_exists, which has no file in the part, so MergeTreeReaderWide::readRows returns 0 and the query exports read_rows = 0 while returning correct data. It normalizes that exported counter from the granule sizes, on the same path where num_rows was already normalized.

@clickhouse-gh clickhouse-gh Bot added the comp-mergetree MergeTree* family: parts, merges, primary index, column statistics, background data transformation. label Sep 11, 2026
@PedroTadim

Copy link
Copy Markdown
Member

cc @tiandiwonder

@clickhouse-gh

clickhouse-gh Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing 6deb19f44 with master 2497bf535 (stripped binary size, per-symbol sizes and ThinLTO time; object sizes against the warmup build of c4bee2f83; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

7 translation units recompiled, 11 s compile time in total, 7 of them have a recent master baseline.

Job report

`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>

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@alexey-milovidov alexey-milovidov self-assigned this Sep 12, 2026
@alexey-milovidov
alexey-milovidov added this pull request to the merge queue Sep 12, 2026
Merged via the queue into ClickHouse:master with commit 36f4981 Sep 12, 2026
174 checks passed
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 12, 2026
ei-grad added a commit to ei-grad/ClickHouse that referenced this pull request Sep 14, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors comp-mergetree MergeTree* family: parts, merges, primary index, column statistics, background data transformation. groeneai-origin-ci-master PR origin: master/nightly CI monitoring finding pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants