Skip to content

Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic#99351

Merged
Avogar merged 3 commits intomasterfrom
fix-check-table-tuple-sparse-dynamic
Mar 17, 2026
Merged

Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic#99351
Avogar merged 3 commits intomasterfrom
fix-check-table-tuple-sparse-dynamic

Conversation

@Avogar
Copy link
Copy Markdown
Member

@Avogar Avogar commented Mar 12, 2026

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC)

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

Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic. Closes #96588

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Note

Medium Risk
Touches core sparse deserialization logic used during reads/validation; small but could affect edge cases in how offsets/state are advanced across granules.

Overview
Fixes SerializationSparse::deserializeOffsets to treat limit=0 (and thus offset+limit==0) as read zero rows instead of behaving like an unbounded read, and simplifies the boundary check logic accordingly.

Adds a stateless regression test covering CHECK TABLE on a Tuple containing a Dynamic element and a sparse-serialized nested tuple, which previously failed with tuple size mismatches.

Written by Cursor Bugbot for commit e405b10. This will update automatically on new commits. Configure here.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 12, 2026

Workflow [PR], commit [e405b10]

Summary:


AI Review

Summary

This PR fixes a real correctness issue in SerializationSparse::deserializeOffsets: limit = 0 now correctly means “read zero rows” instead of accidentally behaving like an unbounded read, and it adds a focused stateless regression test for CHECK TABLE over Tuple(Dynamic, Tuple(Int)) with sparse serialization. I found no blocker/major issues in the patch itself.

Missing context

  • ⚠️ Full CI is still pending, so runtime validation across all test suites is not available yet.

ClickHouse Rules

Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsHistory.cpp
Safe rollout
Compilation time

Final Verdict

  • Status: ✅ Approve

@Algunenano Algunenano self-assigned this Mar 12, 2026
Comment thread src/DataTypes/Serializations/SerializationSparse.cpp Outdated
@rienath
Copy link
Copy Markdown
Member

rienath commented Mar 13, 2026

One of the stress test failures rate might've increased by now reverted #99311. Merging master to remove the noise. The test is still flaky, but chances it will be red are smaller.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Comment thread src/DataTypes/Serializations/SerializationSparse.cpp
@Algunenano Algunenano added pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-critical-bugfix labels Mar 13, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 13, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.70% -0.10%
Functions 23.90% 23.90% +0.00%
Branches 76.30% 76.30% +0.00%

PR changed lines: PR changed-lines coverage: 100.00% (12/12, 0 noise lines excluded)
Diff coverage report
Uncovered code

@Avogar Avogar added this pull request to the merge queue Mar 17, 2026
Merged via the queue into master with commit ccf9cf3 Mar 17, 2026
164 checks passed
@Avogar Avogar deleted the fix-check-table-tuple-sparse-dynamic branch March 17, 2026 11:16
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Mar 17, 2026
robot-clickhouse added a commit that referenced this pull request Mar 17, 2026
robot-clickhouse added a commit that referenced this pull request Mar 17, 2026
robot-clickhouse added a commit that referenced this pull request Mar 17, 2026
robot-clickhouse added a commit that referenced this pull request Mar 17, 2026
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 17, 2026
clickhouse-gh bot added a commit that referenced this pull request Mar 17, 2026
Backport #99351 to 26.2: Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic
Avogar added a commit that referenced this pull request Mar 18, 2026
Backport #99351 to 26.1: Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic
Avogar added a commit that referenced this pull request Mar 18, 2026
Backport #99351 to 25.12: Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic
robot-clickhouse added a commit that referenced this pull request Mar 18, 2026
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Mar 18, 2026
clickhouse-gh bot added a commit that referenced this pull request Mar 19, 2026
Backport #99351 to 25.8: Fix CHECK TABLE with sparse serialization inside Tuple with Dynamic
motsc added a commit to motsc/ClickHouse that referenced this pull request Mar 20, 2026
…tionSparse

Commit e338032 accidentally removed the 'if (max_rows_to_read == 0) return 0'
early return that was introduced by PR ClickHouse#99351 to fix CHECK TABLE on Tuple columns
containing Dynamic elements with sparse-serialized sub-columns (issue ClickHouse#96588).

The removed guard was replaced with 'if (max_rows_to_read && ...)' guards on the
two conditions below, but those do not short-circuit the function: the read loop
still executes when limit=0, reading an extra row instead of returning immediately.
This regresses test 04038_check_table_sparse_tuple_dynamic.

Restore the original early return and remove the spurious 'max_rows_to_read &&'
guard from the loop condition, since it is now unreachable when limit=0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR 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.

Logical error: Unexpected size of tuple element A: B. Expected size: C (STID: 6433-632f)

6 participants