Skip to content

Fix Int32 exponent overflow in SAMPLE ratio parser#109197

Merged
Algunenano merged 2 commits into
ClickHouse:masterfrom
Algunenano:fix-sample-ratio-exponent-overflow
Jul 7, 2026
Merged

Fix Int32 exponent overflow in SAMPLE ratio parser#109197
Algunenano merged 2 commits into
ClickHouse:masterfrom
Algunenano:fix-sample-ratio-exponent-overflow

Conversation

@Algunenano

@Algunenano Algunenano commented Jul 2, 2026

Copy link
Copy Markdown
Member

Closes: #103517

ParserSampleRatio read the exponent of a SAMPLE ratio into an Int32 using the pointer-returning tryReadIntText, which discards the overflow result. On overflow the exponent was left at 0 while the position still advanced, so the only check (whether characters were consumed) passed and a huge or tiny exponent silently became 1e0. For example SELECT count() FROM t SAMPLE 1e-3000000000 sampled 100% of the data instead of ~0 rows.

The exponent is now read via a helper that saturates the magnitude through the existing readUInt128Text path, so any exponent magnitude (not just the Int32/Int64 range) is handled correctly.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix SAMPLE ratios with an exponent whose magnitude overflows Int32 (e.g. SAMPLE 1e-3000000000) being silently treated as SAMPLE 1.

Version info

  • Merged into: 26.7.1.591 (included in 26.7 and later)

@clickhouse-gh

clickhouse-gh Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [a54fce5]

Summary:


AI Review

Summary

This PR fixes ParserSampleRatio so overflowing scientific-notation exponents no longer silently collapse to 1e0, and it now handles _ digit separators consistently across both the mantissa and exponent. The current code matches that contract, the added stateless test covers the original overflow bug plus the grouped-spelling follow-up, and I did not find a remaining correctness or compatibility issue in the live diff.

Missing context / blind spots
  • ⚠️ I could not fetch the Praktika CI report with .claude/tools/fetch_ci_report.js because the current GitHub token is rejected by the enterprise policy, so this review does not include an independent pass over any current CI failures.
Final Verdict

✅ No remaining blockers or majors from this review.

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 2, 2026
Comment thread src/Parsers/ParserSampleRatio.cpp
@clickhouse-gh

clickhouse-gh Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.50% 85.60% +0.10%
Functions 92.70% 92.70% +0.00%
Branches 77.70% 77.70% +0.00%

Changed lines: Changed C/C++ lines covered: 37/38 (97.37%) · Uncovered code

Full report · Diff report

@Algunenano
Algunenano added this pull request to the merge queue Jul 7, 2026
Merged via the queue into ClickHouse:master with commit 28092a4 Jul 7, 2026
344 of 345 checks passed
@Algunenano
Algunenano deleted the fix-sample-ratio-exponent-overflow branch July 7, 2026 11:55
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default 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.

Int32 exponent overflow in parseDecimal causes SAMPLE ratio to silently become 1

2 participants