Fix logical error when parsing incorrect empty tuples by nihalzp · Pull Request #102289 · ClickHouse/ClickHouse · GitHub
Skip to content

Fix logical error when parsing incorrect empty tuples#102289

Merged
nihalzp merged 9 commits into
ClickHouse:masterfrom
nihalzp:fix-empty-tuple-parsing
Apr 20, 2026
Merged

Fix logical error when parsing incorrect empty tuples#102289
nihalzp merged 9 commits into
ClickHouse:masterfrom
nihalzp:fix-empty-tuple-parsing

Conversation

@nihalzp

@nihalzp nihalzp commented Apr 9, 2026 •
edited by robot-clickhouse
Loading

Copy link
Copy Markdown
Member

The following query throws exception:

SELECT CAST('()()', 'Tuple');

Currently, we increment empty tuple column size after addElementSafe's impl has been executed, leading to incorrect column size during validation that happens at the end of impl of SerializationTuple::deserializeTextImpl. Since impl populates the tuple subcolumns, it feels natural that impl should also update the column size.

Reverts #102011.

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 logical error when parsing incorrect empty tuple string.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Version info

  • Merged into: 26.4.1.1067

@clickhouse-gh

clickhouse-gh Bot commented Apr 9, 2026 •
edited by nihalzp
Loading

Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Apr 9, 2026
@nihalzp nihalzp requested a review from Avogar April 10, 2026 02:05
@alexey-milovidov

alexey-milovidov commented Apr 10, 2026

Copy link
Copy Markdown
Member

The flaky check failure is fixed in #102148, let's update the branch.

@Avogar Avogar self-assigned this Apr 10, 2026
Avogar
Avogar requested changes Apr 10, 2026
View reviewed changes

@Avogar Avogar 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.

It's actually better to not use throwUnexpectedDataAfterParsedValue in serializationTuple, because for Tuple we have special logic of rollback in canse of error during parsing. throwUnexpectedDataAfterParsedValue should be used for simple columns with no nested structure that should be rolled back carefully

@nihalzp

nihalzp commented Apr 10, 2026

Copy link
Copy Markdown
Member Author

I can try removing throwUnexpectedDataAfterParsedValue from serializationTuple. Apart from that, as changed in this PR, should impl of addElementSafe have the responsibility to update size via addSize?

@Avogar

Avogar commented Apr 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

should impl of addElementSafe have the responsibility to update size via addSize?

I would say it's ok to keep addSize in addElementSafe and not in impl. Because in case of error in impl we also need to check if we added size of not, with current approach in case of error in impl we know that we didn't add the size

@nihalzp nihalzp requested a review from Avogar April 11, 2026 10:10
@clickhouse-gh

clickhouse-gh Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.00% +0.00%
Functions 90.80% 90.90% +0.10%
Branches 76.50% 76.50% +0.00%

Changed lines: 93.75% (15/16) | lost baseline coverage: 1 line(s) · Uncovered code

Full report · Diff report

Avogar
Avogar approved these changes Apr 13, 2026
View reviewed changes
Comment thread src/DataTypes/Serializations/SerializationTuple.cpp
@nihalzp nihalzp added this pull request to the merge queue Apr 20, 2026
Merged via the queue into ClickHouse:master with commit da17406 Apr 20, 2026
162 of 164 checks passed
@nihalzp nihalzp deleted the fix-empty-tuple-parsing branch April 20, 2026 12:07
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 20, 2026
@groeneai groeneai mentioned this pull request Apr 23, 2026
Merged
1 task
@groeneai groeneai mentioned this pull request May 7, 2026
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Avogar Avogar Avogar approved these changes

Assignees

@Avogar Avogar

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nihalzp @alexey-milovidov @Avogar @robot-clickhouse