fix: quote-aware store-type parser#29
Merged
Merged
Conversation
ExtractInnerTypes and FindMatchingCloseParen counted parens and top-level
commas without tracking whether they were inside a single-quoted string
literal. Enum value names containing parens or commas (e.g.
Enum8(',' = 1, ';' = 2) or 'a)b') would corrupt the depth counter and
either return the wrong close-paren position or split the inner type list
at the wrong place, causing nested types like Array(Enum8(...)) and
Map(String, Enum8(...)) to fail to resolve.
Both helpers now track quote state and treat any paren/comma inside a
'...' literal as opaque content. ClickHouse's doubled-quote escape ('')
for embedding a literal apostrophe inside a string is also handled, so
Enum8('Bob''s' = 1) parses correctly.
Adds a parameterized regression test covering the surrounding-type shapes
(Array, Map, Tuple) crossed with the special-character patterns (closing
paren, opening paren, comma in enum value). Without the fix, three of the
five cases fail; with it, all five resolve correctly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ExtractInnerTypes(viaSplitTopLevel) andFindMatchingCloseParencounted parens and top-level commas without tracking whether they were inside a single-quoted string literal. Enum value names containing parens or commas — e.g.Enum8(',' = 1, ';' = 2)or'a)b'— would corrupt the depth counter and either return the wrong close-paren position or split the inner type list at the wrong place, causing nested types likeArray(Enum8(...))andMap(String, Enum8(...))to fail to resolve.Both helpers now track quote state and treat any paren or top-level comma inside a
'...'literal as opaque content. ClickHouse's doubled-quote escape ('') for embedding a literal apostrophe inside a string is handled too, soEnum8('Bob''s' = 1)parses correctly.The bug is pre-existing — #25's refactor moved the buggy split out of
ExtractInnerTypesinto a namedSplitTopLevelhelper but didn't change the logic.Test plan
FindMapping_NestedEnumWithSpecialChars_ResolvesWithoutCorruptiontheory covers 5 nested-type × special-char combinations (Array,Map,Tuplecrossed with closing-paren, opening-paren, and comma in enum value names). Without the fix, 3 of the 5 cases fail withAssert.NotNullbecause the surrounding parameterized type fails to resolve; with it, all 5 pass.SplitTopLevelextraction)🤖 Generated with Claude Code