Add `readBigStrict` and use it in buffer deserialization by Ergus · Pull Request #107196 · ClickHouse/ClickHouse · GitHub
Skip to content

Add readBigStrict and use it in buffer deserialization#107196

Merged
Avogar merged 2 commits into
ClickHouse:masterfrom
Ergus:add_read_check
Jun 12, 2026
Merged

Add readBigStrict and use it in buffer deserialization#107196
Avogar merged 2 commits into
ClickHouse:masterfrom
Ergus:add_read_check

Conversation

@Ergus

@Ergus Ergus commented Jun 11, 2026

Copy link
Copy Markdown
Member

Add a size match check after readBig

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):

...

Version info

  • Merged into: 26.6.1.707

@Ergus Ergus requested a review from Avogar June 11, 2026 11:49
@clickhouse-gh

clickhouse-gh Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 11, 2026
data.resize(initial_size + bytes_to_read);
stream->ignore(bytes_to_skip);
size_t size = stream->readBig(reinterpret_cast<char*>(&data[initial_size]), bytes_to_read);
if (size != bytes_to_read)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This guard is the behavioral contract of the PR, but there is no regression test for it. Please add a focused test that creates a MergeTree part using string_serialization_version = with_size_stream, truncates the String data stream while leaving the size stream intact, and verifies that reading the part fails with this INCORRECT_DATA path instead of returning a malformed ColumnString or surfacing a later LOGICAL_ERROR. Without coverage, the next readBig refactor can silently reintroduce the partial-read behavior this is fixing.

@Avogar Avogar self-assigned this Jun 11, 2026
@Ergus Ergus changed the title Add size check after readBig Add readBigStrict and use it in buffer deserialization Jun 11, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.60% 84.60% +0.00%
Functions 92.30% 92.30% +0.00%
Branches 77.30% 77.30% +0.00%

Changed lines: Changed C/C++ lines covered by tests: 17/19 (89.47%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

@Avogar Avogar added this pull request to the merge queue Jun 12, 2026
Merged via the queue into ClickHouse:master with commit dabafca Jun 12, 2026
164 of 166 checks passed
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 12, 2026
pull Bot pushed a commit to AKJUS/ClickHouse that referenced this pull request Jun 17, 2026
@clickgapai

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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