Description
Peeling redundant parentheses off a SQL expression is now implemented twice, and scanning a SQL string while ignoring quoted regions is implemented three times. The copies have already drifted apart, and each drift is a real bug (#195, #196).
| Location |
What it does |
Tracks quotes? |
packages/core/src/key-clause.ts — splitTopLevelComma |
split on top-level commas |
' " ` |
packages/core/src/projection.ts — stripWrappingParens |
peel wrapping parens |
' " ` |
packages/plugin-pull/src/index.ts:396 — normalizeWrappedTuple |
peel wrapping parens |
none |
packages/clickhouse/src/create-table-parser.ts — extractCreateTableBody |
find the column-list body |
' " only (#196) |
normalizeWrappedTuple and stripWrappingParens are the same algorithm — same guard (startsWith('(') && endsWith(')')), same depth walk, same "only strip when the leading paren closes at the very end" rule — but they now behave differently:
So (`weird)name`) normalizes correctly in core and incorrectly in plugin-pull, and ((a, b)) peels fully in core but not in pull.
Why it wasn't folded into #193
normalizeWrappedTuple backs splitTopLevelCommaSeparated, which parses key clauses (primaryKey / orderBy) — not projections. Changing it touches the same code path implicated in #194 and #190, so it was deliberately left alone to keep that PR scoped to the reported bug.
Proposed Fix
Export one quote-aware scanning primitive from @chkit/core and have all four call sites use it:
Sequencing note: fixing normalizeWrappedTuple changes how primaryKey/orderBy are parsed on pull, so it is best done alongside #194/#190 with their tests in place, rather than as a standalone refactor.
Verification
- The paren/quote behavior table above becomes a single shared unit test rather than one per copy.
packages/plugin-pull key-clause parsing is unchanged for the cases it already handles — assert against existing pull tests before and after.
- No behavior change to projections (covered by
packages/core/src/index.test.ts).
Found while verifying #183 / PR #193. Related: #194, #195, #196.
Description
Peeling redundant parentheses off a SQL expression is now implemented twice, and scanning a SQL string while ignoring quoted regions is implemented three times. The copies have already drifted apart, and each drift is a real bug (#195, #196).
packages/core/src/key-clause.ts—splitTopLevelComma'"`packages/core/src/projection.ts—stripWrappingParens'"`packages/plugin-pull/src/index.ts:396—normalizeWrappedTuplepackages/clickhouse/src/create-table-parser.ts—extractCreateTableBody'"only (#196)normalizeWrappedTupleandstripWrappingParensare the same algorithm — same guard (startsWith('(') && endsWith(')')), same depth walk, same "only strip when the leading paren closes at the very end" rule — but they now behave differently:stripWrappingParensis quote-aware and peels every redundant layer (PR fix(pull): capture index-only projections and render them correctly #193 — peeling only one layer was non-idempotent and made everygeneratere-emit a drop + rebuild).normalizeWrappedTupleis neither. It peels a single layer and miscounts parens inside quoted identifiers.So
(`weird)name`)normalizes correctly in core and incorrectly in plugin-pull, and((a, b))peels fully in core but not in pull.Why it wasn't folded into #193
normalizeWrappedTuplebackssplitTopLevelCommaSeparated, which parses key clauses (primaryKey/orderBy) — not projections. Changing it touches the same code path implicated in #194 and #190, so it was deliberately left alone to keep that PR scoped to the reported bug.Proposed Fix
Export one quote-aware scanning primitive from
@chkit/coreand have all four call sites use it:splitTopLevelComma(already exported) stays as-is.stripWrappingParens(or apeelRedundantParens) and deletenormalizeWrappedTuplein favour of it.extractCreateTableBodyto use the shared quote handling (bug: create-table parser does not track backtick identifiers, silently dropping projections on tables with parens in a column name #196).Sequencing note: fixing
normalizeWrappedTuplechanges howprimaryKey/orderByare parsed on pull, so it is best done alongside #194/#190 with their tests in place, rather than as a standalone refactor.Verification
packages/plugin-pullkey-clause parsing is unchanged for the cases it already handles — assert against existing pull tests before and after.packages/core/src/index.test.ts).Found while verifying #183 / PR #193. Related: #194, #195, #196.