diff --git a/.changeset/consolidate-quote-aware-sql-scanning.md b/.changeset/consolidate-quote-aware-sql-scanning.md new file mode 100644 index 0000000..ca23313 --- /dev/null +++ b/.changeset/consolidate-quote-aware-sql-scanning.md @@ -0,0 +1,9 @@ +--- +"@chkit/clickhouse": patch +"@chkit/plugin-pull": patch +"@chkit/core": patch +--- + +Fix `chkit pull` silently dropping a table's projections (and mangling its `ORDER BY`/`PRIMARY KEY`) when a column has a backtick-quoted name containing a parenthesis, e.g. `` `weird)name` `` (#196). The create-table body scanner counted that paren as structure, truncating the parse. + +The root cause was four separate copies of "scan SQL while ignoring quoted regions", which had already drifted — one tracked no quotes at all, another missed backticks (#197). They now share a single quote-aware primitive (`nextQuote`) in `@chkit/core`, alongside shared `stripWrappingParens` and `findMatchingParen` helpers, so the rule lives in one place and every scanner (`splitTopLevelComma`, the projection index normalizer, the pull key-clause parser, and the create-table body finder) handles single-quoted strings, double-quoted identifiers, and backtick identifiers identically. diff --git a/packages/clickhouse/src/create-table-parser.ts b/packages/clickhouse/src/create-table-parser.ts index 09fbed9..358c619 100644 --- a/packages/clickhouse/src/create-table-parser.ts +++ b/packages/clickhouse/src/create-table-parser.ts @@ -1,4 +1,9 @@ -import { normalizeProjectionIndex, normalizeSQLFragment, splitTopLevelComma } from '@chkit/core' +import { + findMatchingParen, + normalizeProjectionIndex, + normalizeSQLFragment, + splitTopLevelComma, +} from '@chkit/core' type ProjectionDefinitionShape = | { name: string; query: string } @@ -32,40 +37,14 @@ function parseClauseFromCreateTableQuery( function findColumnListBounds( createTableQuery: string ): { open: number; close: number } | undefined { + // Require a table-level `) ENGINE =` so we don't treat some other parenthesised + // fragment as the column list. const engineMatch = /\)\s*ENGINE\s*=/i.exec(createTableQuery) if (!engineMatch || engineMatch.index === undefined) return undefined - const left = createTableQuery.slice(0, engineMatch.index + 1) - const openIndex = left.indexOf('(') + const openIndex = createTableQuery.indexOf('(') if (openIndex === -1) return undefined - - let depth = 0 - let inString = false - let stringQuote = "'" - for (let i = openIndex; i < left.length; i += 1) { - const char = left[i] - if (!char) continue - if (inString) { - if (char === stringQuote && left[i - 1] !== '\\') { - inString = false - } - continue - } - if (char === "'" || char === '"') { - inString = true - stringQuote = char - continue - } - if (char === '(') { - depth += 1 - continue - } - if (char === ')') { - depth -= 1 - if (depth === 0) return { open: openIndex, close: i } - } - } - - return undefined + const close = findMatchingParen(createTableQuery, openIndex) + return close === undefined ? undefined : { open: openIndex, close } } function extractCreateTableBody(createTableQuery: string | undefined): string | undefined { diff --git a/packages/clickhouse/src/index.test.ts b/packages/clickhouse/src/index.test.ts index 1dff8ed..f9a7fb6 100644 --- a/packages/clickhouse/src/index.test.ts +++ b/packages/clickhouse/src/index.test.ts @@ -503,6 +503,23 @@ ORDER BY a` expect(parseOrderByFromCreateTableQuery(query)).toBe('id') expect(parseSettingsFromCreateTableQuery(query)).toEqual({ index_granularity: '8192' }) }) + + // Regression for #196: a backtick-quoted column name containing a paren used + // to unbalance the body scan and truncate the parse, dropping the projection. + test('handles a backtick column name containing a paren', () => { + const query = `CREATE TABLE app.events (\`id\` UInt64, \`weird)name\` String, PROJECTION p INDEX id TYPE basic) ENGINE = MergeTree ORDER BY id` + + expect(parseProjectionsFromCreateTableQuery(query)).toEqual([ + { name: 'p', index: 'id', type: 'basic' }, + ]) + expect(parseOrderByFromCreateTableQuery(query)).toBe('id') + }) + + test('keeps backtick identifiers intact inside key clauses', () => { + const query = `CREATE TABLE app.events (\`id\` UInt64, \`w)x\` String) ENGINE = MergeTree ORDER BY (\`w)x\`, id)` + + expect(parseOrderByFromCreateTableQuery(query)).toBe('(`w)x`, id)') + }) }) describe('formatConnectionError', () => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 228c789..b43e6eb 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -9,6 +9,7 @@ export { export { planDiff } from './planner.js' export { createSnapshot } from './snapshot.js' export { splitTopLevelComma } from './key-clause.js' +export { findMatchingParen, stripWrappingParens } from './sql-scan.js' export { isIndexProjection, normalizeProjectionIndex } from './projection.js' export { normalizeEngine, normalizeSQLFragment } from './sql-normalizer.js' export { toCreateSQL } from './sql.js' diff --git a/packages/core/src/key-clause.ts b/packages/core/src/key-clause.ts index 7de7f3c..29f9c3e 100644 --- a/packages/core/src/key-clause.ts +++ b/packages/core/src/key-clause.ts @@ -1,3 +1,5 @@ +import { nextQuote } from './sql-scan.js' + export function splitTopLevelComma(input: string): string[] { const out: string[] = [] let current = '' @@ -6,16 +8,12 @@ export function splitTopLevelComma(input: string): string[] { for (let i = 0; i < input.length; i += 1) { const char = input[i] ?? '' - const prev = i > 0 ? input[i - 1] : '' - - if (quote) { - current += char - if (char === quote && prev !== '\\') quote = null - continue - } + const prev = i > 0 ? (input[i - 1] ?? '') : '' + const quoteBefore = quote + quote = nextQuote(char, prev, quote) - if (char === "'" || char === '"' || char === '`') { - quote = char + // A char inside a quoted literal (body or delimiter) is never structural. + if (quoteBefore !== null || quote !== null) { current += char continue } diff --git a/packages/core/src/projection.ts b/packages/core/src/projection.ts index a0dd915..2046b32 100644 --- a/packages/core/src/projection.ts +++ b/packages/core/src/projection.ts @@ -1,5 +1,6 @@ import type { IndexProjectionDefinition, ProjectionDefinition } from './model-types.js' import { splitTopLevelComma } from './key-clause.js' +import { nextQuote, stripWrappingParens } from './sql-scan.js' import { normalizeSQLFragment } from './sql-normalizer.js' export function isIndexProjection( @@ -8,47 +9,16 @@ export function isIndexProjection( return 'index' in projection } -function stripWrappingParens(input: string): string { - if (!input.startsWith('(') || !input.endsWith(')')) return input - - // Only strip when the leading paren closes at the very end, so `(a), (b)` - // keeps both groups. Parens inside quoted identifiers and string literals - // are text, not nesting — `` (`weird)name`) `` is still a single wrapped - // expression. - let depth = 0 - let quote: "'" | '"' | '`' | null = null - for (let i = 0; i < input.length; i += 1) { - const char = input[i] - if (quote) { - if (char === quote && input[i - 1] !== '\\') quote = null - continue - } - if (char === "'" || char === '"' || char === '`') { - quote = char - continue - } - if (char === '(') depth += 1 - else if (char === ')') { - depth -= 1 - if (depth === 0) return i === input.length - 1 ? input.slice(1, -1).trim() : input - } - } - return input -} - /** ClickHouse prints one space after every argument separator. */ function spaceAfterCommas(input: string): string { let out = '' let quote: "'" | '"' | '`' | null = null for (let i = 0; i < input.length; i += 1) { const char = input[i] ?? '' - if (quote) { - out += char - if (char === quote && input[i - 1] !== '\\') quote = null - continue - } - if (char === "'" || char === '"' || char === '`') { - quote = char + const prev = i > 0 ? (input[i - 1] ?? '') : '' + const quoteBefore = quote + quote = nextQuote(char, prev, quote) + if (quoteBefore !== null || quote !== null) { out += char continue } diff --git a/packages/core/src/sql-scan.test.ts b/packages/core/src/sql-scan.test.ts new file mode 100644 index 0000000..9522b8e --- /dev/null +++ b/packages/core/src/sql-scan.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, test } from 'bun:test' + +import { splitTopLevelComma } from './key-clause.js' +import { findMatchingParen, stripWrappingParens } from './sql-scan.js' + +describe('splitTopLevelComma', () => { + test('splits only at top-level commas', () => { + expect(splitTopLevelComma('a, b, c')).toEqual(['a', 'b', 'c']) + expect(splitTopLevelComma('cityHash64(a, b), c')).toEqual(['cityHash64(a, b)', 'c']) + }) + + test('ignores commas and parens inside quotes of every kind', () => { + expect(splitTopLevelComma("'x,y', z")).toEqual(["'x,y'", 'z']) + expect(splitTopLevelComma('"x,y", z')).toEqual(['"x,y"', 'z']) + expect(splitTopLevelComma('`a,b`, c')).toEqual(['`a,b`', 'c']) + expect(splitTopLevelComma('`a)b`, c')).toEqual(['`a)b`', 'c']) + }) +}) + +describe('stripWrappingParens', () => { + test('peels exactly one wrapping layer, and only a genuine wrapper', () => { + expect(stripWrappingParens('(a, b)')).toBe('a, b') + expect(stripWrappingParens('((a, b))')).toBe('(a, b)') + expect(stripWrappingParens('(a), (b)')).toBe('(a), (b)') + expect(stripWrappingParens('a, b')).toBe('a, b') + }) + + test('is not fooled by a paren inside a backtick identifier', () => { + expect(stripWrappingParens('(`w)x`)')).toBe('`w)x`') + expect(stripWrappingParens('(`w)x`, a)')).toBe('`w)x`, a') + }) +}) + +describe('findMatchingParen', () => { + test('finds the close matching the open at the given index', () => { + expect(findMatchingParen('(a, b) rest', 0)).toBe(5) + expect(findMatchingParen('f(g(x)) rest', 1)).toBe(6) + }) + + test('ignores parens inside quotes', () => { + // The ')' inside the quoted region must not close the group. + expect(findMatchingParen('(`w)x`) tail', 0)).toBe(6) + expect(findMatchingParen("('a)b') tail", 0)).toBe(6) + }) + + test('returns undefined when unbalanced', () => { + expect(findMatchingParen('(a, b', 0)).toBeUndefined() + }) +}) diff --git a/packages/core/src/sql-scan.ts b/packages/core/src/sql-scan.ts new file mode 100644 index 0000000..9103f24 --- /dev/null +++ b/packages/core/src/sql-scan.ts @@ -0,0 +1,74 @@ +// Quote-aware SQL scanning primitives. Characters inside a single-quoted +// string, a double-quoted identifier, or a backtick-quoted identifier are +// literal text, not structure — parens and commas there must be ignored. +// +// Every scanner in the codebase shares `nextQuote` so the rule lives in exactly +// one place and the copies cannot drift apart again (#197). Missing backtick +// tracking here is what let a column named `weird)name` truncate a parsed +// table body (#196). + +type QuoteChar = "'" | '"' | '`' + +/** + * The quote state after reading `char`, given the previous character and the + * state before it. `null` means "not inside a quoted literal". A quote closes + * on a matching, unescaped quote char; ClickHouse's doubled-quote escaping + * (`''`) falls out correctly as a close immediately followed by a re-open. + */ +export function nextQuote(char: string, prevChar: string, quote: QuoteChar | null): QuoteChar | null { + if (quote) return char === quote && prevChar !== '\\' ? null : quote + if (char === "'" || char === '"' || char === '`') return char + return null +} + +/** True when `char` is part of a quoted literal (its body or its delimiters). */ +function isQuoted(char: string, prevChar: string, quoteBefore: QuoteChar | null): boolean { + return quoteBefore !== null || nextQuote(char, prevChar, quoteBefore) !== null +} + +/** + * Peel one layer of wrapping parentheses, but only when the leading `(` closes + * at the very end — so `(a, b)` becomes `a, b` while `(a), (b)` is left intact. + */ +export function stripWrappingParens(input: string): string { + if (!input.startsWith('(') || !input.endsWith(')')) return input + + let depth = 0 + let quote: QuoteChar | null = null + for (let i = 0; i < input.length; i += 1) { + const char = input[i] ?? '' + const prev = i > 0 ? (input[i - 1] ?? '') : '' + const quoted = isQuoted(char, prev, quote) + quote = nextQuote(char, prev, quote) + if (quoted) continue + if (char === '(') depth += 1 + else if (char === ')') { + depth -= 1 + if (depth === 0) return i === input.length - 1 ? input.slice(1, -1).trim() : input + } + } + return input +} + +/** + * Index of the `)` that matches the `(` at `openIndex`, or undefined when the + * parens are unbalanced. Quote-aware, so parens inside identifiers/strings + * don't count. + */ +export function findMatchingParen(input: string, openIndex: number): number | undefined { + let depth = 0 + let quote: QuoteChar | null = null + for (let i = openIndex; i < input.length; i += 1) { + const char = input[i] ?? '' + const prev = i > 0 ? (input[i - 1] ?? '') : '' + const quoted = isQuoted(char, prev, quote) + quote = nextQuote(char, prev, quote) + if (quoted) continue + if (char === '(') depth += 1 + else if (char === ')') { + depth -= 1 + if (depth === 0) return i + } + } + return undefined +} diff --git a/packages/plugin-pull/src/index.ts b/packages/plugin-pull/src/index.ts index dbb6be0..db9d588 100644 --- a/packages/plugin-pull/src/index.ts +++ b/packages/plugin-pull/src/index.ts @@ -19,6 +19,7 @@ import { type SafeParseable, type SchemaDefinition, splitTopLevelComma, + stripWrappingParens, type TableDefinition, withFactoryDefaults, } from '@chkit/core' @@ -390,26 +391,7 @@ function summarizeSkippedObjects( function splitTopLevelCommaSeparated(input: string | undefined): string[] { if (!input) return [] - return splitTopLevelComma(input).map(normalizeWrappedTuple) -} - -function normalizeWrappedTuple(input: string): string { - const trimmed = input.trim() - if (!trimmed.startsWith('(') || !trimmed.endsWith(')')) { - return trimmed - } - - let depth = 0 - for (let i = 0; i < trimmed.length; i += 1) { - const char = trimmed[i] - if (char === '(') depth += 1 - if (char === ')') depth -= 1 - if (depth === 0 && i < trimmed.length - 1) { - return trimmed - } - } - - return trimmed.slice(1, -1).trim() + return splitTopLevelComma(input).map(stripWrappingParens) } async function listNonTableRows(