From 116a556c3aa666c9545435cb3238c236632e9c56 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 25 Jun 2026 22:58:44 +0200 Subject: [PATCH 1/2] fix: backtick-quote non-bare object names everywhere SQL is generated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Object names that aren't bare identifiers — dashes, dots, spaces, e.g. target_all.`part-00000-…-c000.snappy.parquet` — were emitted unquoted, so double-click (SELECT *), shift-click / node-click (SHOW CREATE), drag-to-insert, and autocomplete all produced SYNTAX_ERROR. Add pure helpers quoteIdent / qualifyIdent to core/format.js (backtick + escape only when a name isn't /^[A-Za-z_]\w*$/) and apply them at every site that turns an object name into SQL or editor text: - ui/schema.js — SELECT * FROM, SHOW CREATE (table + DATABASE), db/table/column drag identifiers, db double-click insert, column insert/cast. Keeps the raw `key` for internal identity (Sets, dbl-click tracking). - ui/explain-graph.js — schema-graph node click (split id on the first dot, quote each part). - core/completions.js — db/table/column completion `insert` values (label stays the bare name). Also fix core/schema-graph.js graph linking: dependencies_* / engine-arg / dict-source references carry the db separately, so join them unconditionally (new joinId) instead of via the dot heuristic — a dotted table name kept its db prefix instead of being mistaken for an already-qualified ref. The remaining heuristic (qualify) is now used only for MV-target / EXPLAIN-AST names that may genuinely be db-qualified, where a miss drops an edge rather than mis-linking. 866 tests pass; format.js + completions.js at 100%, gate holds. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu --- src/core/completions.js | 9 ++++--- src/core/format.js | 25 ++++++++++++++++++ src/core/schema-graph.js | 16 ++++++++---- src/ui/explain-graph.js | 8 ++++-- src/ui/schema.js | 23 +++++++++-------- tests/unit/explain-graph.test.js | 8 ++++++ tests/unit/format.test.js | 29 ++++++++++++++++++++- tests/unit/schema-graph.test.js | 11 ++++++++ tests/unit/schema.test.js | 44 ++++++++++++++++++++++++++++++++ 9 files changed, 151 insertions(+), 22 deletions(-) diff --git a/src/core/completions.js b/src/core/completions.js index 1e64c12..0fa2204 100644 --- a/src/core/completions.js +++ b/src/core/completions.js @@ -7,6 +7,7 @@ // built-in tokenizer sets when the server didn't supply them. import { SQL_KEYWORDS, SQL_FUNCS } from './sql-highlight.js'; +import { quoteIdent } from './format.js'; const BUILTIN_KEYWORDS = [...SQL_KEYWORDS]; const BUILTIN_FUNCS = [...SQL_FUNCS]; @@ -93,12 +94,14 @@ export function buildCompletions(ref, schema) { items.push({ label: name, kind: 'format', insert: name, detail: 'format' }); } for (const db of schema || []) { - items.push({ label: db.db, kind: 'db', insert: db.db, detail: 'database' }); + // `label` shows the bare name; `insert` is the SQL-safe (backtick-quoted when + // needed) identifier so a `…snappy.parquet` table inserts as valid SQL. + items.push({ label: db.db, kind: 'db', insert: quoteIdent(db.db), detail: 'database' }); for (const tb of db.tables || []) { - items.push({ label: tb.name, kind: 'table', insert: tb.name, detail: 'table', parent: db.db }); + items.push({ label: tb.name, kind: 'table', insert: quoteIdent(tb.name), detail: 'table', parent: db.db }); if (Array.isArray(tb.columns)) { for (const c of tb.columns) { - items.push({ label: c.name, kind: 'column', insert: c.name, detail: c.type, parent: tb.name }); + items.push({ label: c.name, kind: 'column', insert: quoteIdent(c.name), detail: c.type, parent: tb.name }); } } } diff --git a/src/core/format.js b/src/core/format.js index 2621e80..16eb8de 100644 --- a/src/core/format.js +++ b/src/core/format.js @@ -50,6 +50,31 @@ export function sqlString(s) { return "'" + String(s).replace(/\\/g, '\\\\').replace(/'/g, "''") + "'"; } +// A bare (unquoted) ClickHouse identifier: a letter/underscore then word chars. +// Anything else (dashes, dots, spaces — e.g. a `…snappy.parquet` table) MUST be +// backtick-quoted or it's a syntax error. +const BARE_IDENT = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * Quote `name` as a ClickHouse identifier when it isn't a bare identifier: + * backticks, with `\` and `` ` `` backslash-escaped (CH's identifier escaping). + * Bare identifiers pass through unquoted so ordinary SQL stays readable. + */ +export function quoteIdent(name) { + const s = String(name); + if (BARE_IDENT.test(s)) return s; + return '`' + s.replace(/\\/g, '\\\\').replace(/`/g, '\\`') + '`'; +} + +/** + * Join already-separate identifier parts into a dotted reference, quoting each + * part as needed: `qualifyIdent('db', 'a.b')` → `` db.`a.b` ``. Empty/nullish + * parts are dropped (so a bare table name qualifies to just itself). + */ +export function qualifyIdent(...parts) { + return parts.filter((p) => p != null && p !== '').map(quoteIdent).join('.'); +} + /** * Terminate `sql` so a programmatic full-replace (Format / Insert DDL) leaves the * caret on empty space rather than at the end of the last token. The editor's diff --git a/src/core/schema-graph.js b/src/core/schema-graph.js index 2a9a4ec..cbfb6e0 100644 --- a/src/core/schema-graph.js +++ b/src/core/schema-graph.js @@ -71,9 +71,15 @@ export function parseEngineRef(engine, engineFull) { return null; } -// A *reference* may already be `db.table` or a bare `table`; an actual row's id is -// always `database.name` (table names like `.inner_id.` contain dots). +// A *reference* whose name MIGHT already be `db.table` (MV target / EXPLAIN AST +// source) — the dot heuristic decides. Ambiguous for table names that themselves +// contain dots, but those paths only ever *match against* known ids, so a miss +// drops an edge rather than mis-linking. const qualify = (db, name) => (name && name.includes('.') ? name : db + '.' + name); +// A reference whose db is always supplied separately (dependencies_*, engine +// args) — join unconditionally so a dotted table name (`…snappy.parquet`) keeps +// its db prefix instead of being mistaken for an already-qualified ref. +const joinId = (db, name) => (db ? db + '.' + name : name); const rowId = (r) => r.database + '.' + r.name; /** @@ -129,7 +135,7 @@ export function buildSchemaGraph(rows, focus) { seen.add(k); edges.push({ from, to, kind }); }; - const zip = (dbs, names) => (names || []).map((nm, i) => qualify((dbs && dbs[i]) || '', nm)); + const zip = (dbs, names) => (names || []).map((nm, i) => joinId((dbs && dbs[i]) || '', nm)); for (const t of tables) { const id = rowId(t); @@ -154,7 +160,7 @@ export function buildSchemaGraph(rows, focus) { } else if (kind === 'distributed' || kind === 'buffer' || kind === 'merge') { const ref = parseEngineRef(t.engine, t.engine_full); if (ref && ref.table) { - const refId = qualify(ref.db || t.database, ref.table); + const refId = joinId(ref.db || t.database, ref.table); node(refId, byId.has(refId) ? nodes.get(refId).kind : 'table'); addEdge(refId, id, ref.kind === 'buffer' ? 'buffer' : 'shard'); } else if (ref && ref.regex) { @@ -179,7 +185,7 @@ export function buildSchemaGraph(rows, focus) { for (const src of ld) { node(src, byId.has(src) ? nodes.get(src).kind : 'table'); addEdge(src, id, 'dict'); } } else { const s = parseDictSource(d && d.source, t.create_table_query); - if (s && s.table) { const sid = qualify(s.db || t.database, s.table); node(sid, 'table'); addEdge(sid, id, 'dict'); } + if (s && s.table) { const sid = joinId(s.db || t.database, s.table); node(sid, 'table'); addEdge(sid, id, 'dict'); } else if (s && s.external) addEdge(external(s.external), id, 'dict'); } } diff --git a/src/ui/explain-graph.js b/src/ui/explain-graph.js index 930c7c1..df61765 100644 --- a/src/ui/explain-graph.js +++ b/src/ui/explain-graph.js @@ -9,6 +9,7 @@ import { h, s } from './dom.js'; import { Icon } from './icons.js'; import { parseDot } from '../core/dot.js'; import { dagreLayout } from '../core/dot-layout.js'; +import { quoteIdent, qualifyIdent } from '../core/format.js'; import { fitBox, zoomBox, panBox, viewBoxStr } from '../core/panzoom.js'; const ZOOM_STEP = 1.2; // per wheel notch / button press @@ -195,11 +196,14 @@ export function openPipelineFullscreen(app, rawText) { } // Clicking an object runs SHOW CREATE for it, dropping the (formatted) DDL into -// the editor — the same action as a shift-click in the schema tree. External +// the editor — the same action as a shift-click in the schema tree. The id is +// `db.name` (split on the FIRST dot — a table name may itself contain dots); each +// part is quoted so non-bare names (`…snappy.parquet`) stay valid SQL. External // dictionary-source leaves have no DDL. const schemaClick = (app) => (n) => { if (!n.id || n.id.startsWith('ext:')) return; - app.actions.insertCreate(n.id); + const dot = n.id.indexOf('.'); + app.actions.insertCreate(dot < 0 ? quoteIdent(n.id) : qualifyIdent(n.id.slice(0, dot), n.id.slice(dot + 1))); }; /** Fullscreen schema-lineage graph. */ diff --git a/src/ui/schema.js b/src/ui/schema.js index 46582c6..c29f127 100644 --- a/src/ui/schema.js +++ b/src/ui/schema.js @@ -3,7 +3,7 @@ import { h } from './dom.js'; import { Icon } from './icons.js'; -import { formatRows } from '../core/format.js'; +import { formatRows, quoteIdent, qualifyIdent } from '../core/format.js'; import { IDENT_MIME, SCHEMA_GRAPH_MIME } from './editor.js'; // Make a tree row a drag source carrying `text` as the schema identifier, so it @@ -78,12 +78,12 @@ export function renderSchema(app) { class: 'tree-row bold', title: 'Click to expand · double-click to insert · shift-click for SHOW CREATE', onclick: (e) => { - if (e.shiftKey) { app.actions.insertCreate('DATABASE ' + db.db); return; } - if (isDoubleClick(app, 'db:' + db.db)) { app.actions.insertAtCursor(db.db); return; } + if (e.shiftKey) { app.actions.insertCreate('DATABASE ' + quoteIdent(db.db)); return; } + if (isDoubleClick(app, 'db:' + db.db)) { app.actions.insertAtCursor(quoteIdent(db.db)); return; } db.expanded = !db.expanded; renderSchema(app); }, - ...lineageDrag(db.db, { kind: 'db', db: db.db }), + ...lineageDrag(quoteIdent(db.db), { kind: 'db', db: db.db }), }, ...treeRow(Icon.database(), db.db, String(db.tables.length), { expanded: db.expanded }), )); @@ -94,7 +94,8 @@ export function renderSchema(app) { const colsHave = Array.isArray(tb.columns) ? tb.columns : []; const visibleCols = filter ? colsHave.filter((c) => matches(c.name)) : colsHave; if (filter && !tableMatch && visibleCols.length === 0 && tb.columns !== 'loading') continue; - const key = db.db + '.' + tb.name; + const key = db.db + '.' + tb.name; // internal identity (Sets, dbl-click tracking) + const qname = qualifyIdent(db.db, tb.name); // SQL-safe qualified name const isOpen = state.expandedTables.has(key); const tbComment = (tb.comment || '').trim(); const title = tbComment @@ -105,10 +106,10 @@ export function renderSchema(app) { class: 'tree-row' + (filter && tableMatch ? ' match' : ''), style: { paddingLeft: '24px' }, title, - ...lineageDrag(key, { kind: 'table', db: db.db, table: tb.name }), + ...lineageDrag(qname, { kind: 'table', db: db.db, table: tb.name }), onclick: (e) => { - if (e.shiftKey) { app.actions.insertCreate(key); return; } - if (isDoubleClick(app, 'tb:' + key)) { app.actions.replaceEditor('SELECT * FROM ' + key + ' LIMIT 100'); return; } + if (e.shiftKey) { app.actions.insertCreate(qname); return; } + if (isDoubleClick(app, 'tb:' + key)) { app.actions.replaceEditor('SELECT * FROM ' + qname + ' LIMIT 100'); return; } if (state.expandedTables.has(key)) state.expandedTables.delete(key); else state.expandedTables.add(key); if (state.expandedTables.has(key) && tb.columns == null) app.actions.loadColumns(db.db, tb.name, tb); @@ -134,10 +135,10 @@ export function renderSchema(app) { || ('Double-click or drag to insert ' + c.name + ' · shift-click for ' + c.name + '::' + c.type), onclick: (e) => { e.stopPropagation(); - if (e.shiftKey) { app.actions.insertAtCursor(c.name + '::' + c.type); return; } - if (isDoubleClick(app, 'col:' + key + '.' + c.name)) app.actions.insertAtCursor(c.name); + if (e.shiftKey) { app.actions.insertAtCursor(quoteIdent(c.name) + '::' + c.type); return; } + if (isDoubleClick(app, 'col:' + key + '.' + c.name)) app.actions.insertAtCursor(quoteIdent(c.name)); }, - ...dragProps(c.name), + ...dragProps(quoteIdent(c.name)), }, ...treeRow(Icon.col(), c.name, c.type, { expanded: null, iconColor: 'var(--fg-faint)' }), )); diff --git a/tests/unit/explain-graph.test.js b/tests/unit/explain-graph.test.js index 06978b5..4ac501f 100644 --- a/tests/unit/explain-graph.test.js +++ b/tests/unit/explain-graph.test.js @@ -194,6 +194,14 @@ describe('schema lineage graph', () => { expect(actions.insertCreate).toHaveBeenCalledWith('lin.mv'); }); + it('clicking a node with a non-bare name backtick-quotes the SHOW CREATE target', () => { + const actions = { insertCreate: vi.fn() }; + const g = { focus: { kind: 'db', db: 'target_all' }, nodes: [{ id: 'target_all.a-b.parquet', label: 'a-b.parquet', kind: 'table' }], edges: [] }; + const el = renderSchemaGraph({ document, Dagre: dagre, actions }, { schemaGraph: g }); + el.querySelector('rect.eg-node--table').dispatchEvent(new Event('click', { bubbles: true })); + expect(actions.insertCreate).toHaveBeenCalledWith('target_all.`a-b.parquet`'); + }); + it('a plain drag does not pan (click selects); ⌘/Ctrl-drag pans', () => { const el = renderSchemaGraph({ document, Dagre: dagre, actions: { insertCreate: vi.fn() } }, { schemaGraph: GRAPH }); const svg = el.querySelector('svg.explain-graph'); diff --git a/tests/unit/format.test.js b/tests/unit/format.test.js index f65dd36..982de01 100644 --- a/tests/unit/format.test.js +++ b/tests/unit/format.test.js @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; import { - clamp, formatRows, formatBytes, timeAgo, sqlString, inferQueryName, isNumericType, shortVersion, userShortName, withStatementBreak, detectSqlFormat, toSubquery, + clamp, formatRows, formatBytes, timeAgo, sqlString, quoteIdent, qualifyIdent, inferQueryName, isNumericType, shortVersion, userShortName, withStatementBreak, detectSqlFormat, toSubquery, } from '../../src/core/format.js'; describe('clamp', () => { @@ -69,6 +69,33 @@ describe('sqlString', () => { }); }); +describe('quoteIdent', () => { + it('leaves a bare identifier unquoted', () => { + expect(quoteIdent('users')).toBe('users'); + expect(quoteIdent('_x9')).toBe('_x9'); + }); + it('backtick-quotes names with non-identifier chars', () => { + expect(quoteIdent('part-00000-c000.snappy.parquet')).toBe('`part-00000-c000.snappy.parquet`'); + expect(quoteIdent('has space')).toBe('`has space`'); + expect(quoteIdent('9starts')).toBe('`9starts`'); // leading digit isn't bare + }); + it('escapes backslashes and backticks inside the quotes', () => { + expect(quoteIdent('a`b')).toBe('`a\\`b`'); + expect(quoteIdent('a\\b')).toBe('`a\\\\b`'); + }); +}); + +describe('qualifyIdent', () => { + it('quotes each part and joins with a dot', () => { + expect(qualifyIdent('db', 'tbl')).toBe('db.tbl'); + expect(qualifyIdent('target_all', 'part-0.snappy.parquet')).toBe('target_all.`part-0.snappy.parquet`'); + }); + it('drops empty/nullish parts (a bare name qualifies to itself)', () => { + expect(qualifyIdent('', 'tbl')).toBe('tbl'); + expect(qualifyIdent(null, 'a-b')).toBe('`a-b`'); + }); +}); + describe('withStatementBreak', () => { it('appends a newline so the caret clears the last token', () => { expect(withStatementBreak('SELECT 1')).toBe('SELECT 1\n'); diff --git a/tests/unit/schema-graph.test.js b/tests/unit/schema-graph.test.js index 9941fc0..0db051a 100644 --- a/tests/unit/schema-graph.test.js +++ b/tests/unit/schema-graph.test.js @@ -195,6 +195,17 @@ describe('buildSchemaGraph', () => { expect(() => buildSchemaGraph(rows, { kind: 'db', db: 'lin' })).not.toThrow(); }); + it('keeps the db prefix for a dependency whose table name contains dots', () => { + // dependencies_* carry the db separately, so a dotted table name (a parquet + // file table) must still join to db., not be mistaken for db-qualified. + const rows = { tables: [ + T('target_all', 'part-0.snappy.parquet', 'MergeTree', { dependencies_database: ['target_all'], dependencies_table: ['v_over_parquet'] }), + T('target_all', 'v_over_parquet', 'View'), + ], dictionaries: [] }; + const g = buildSchemaGraph(rows, { kind: 'db', db: 'target_all' }); + expect(eset(g).has('target_all.part-0.snappy.parquet>target_all.v_over_parquet:feeds')).toBe(true); + }); + it('tolerates empty input', () => { expect(buildSchemaGraph(null, { kind: 'db', db: 'x' })).toEqual({ nodes: [], edges: [] }); expect(buildSchemaGraph({}, null)).toEqual({ nodes: [], edges: [] }); diff --git a/tests/unit/schema.test.js b/tests/unit/schema.test.js index 29019cb..4cbf459 100644 --- a/tests/unit/schema.test.js +++ b/tests/unit/schema.test.js @@ -218,6 +218,50 @@ describe('renderSchema drag sources', () => { }); }); +describe('renderSchema with non-bare object names (backtick quoting)', () => { + const PARQUET = 'part-00000-70041866.snappy.parquet'; + function withParquet() { + const app = makeApp(); + app.state.schema = [{ + db: 'target_all', expanded: true, + tables: [{ name: PARQUET, total_rows: '1', total_bytes: '1', comment: '', columns: null }], + }]; + return app; + } + const tbRow = (app) => rows(app).find((r) => r.querySelector('.label').textContent === PARQUET); + + it('double-click → SELECT * quotes the dotted/dashed table name', () => { + const app = withParquet(); + renderSchema(app); + dblclick(tbRow(app)); + expect(app.actions.replaceEditor).toHaveBeenCalledWith('SELECT * FROM target_all.`' + PARQUET + '` LIMIT 100'); + }); + it('shift-click → SHOW CREATE target is backtick-quoted', () => { + const app = withParquet(); + renderSchema(app); + shiftClick(tbRow(app)); + expect(app.actions.insertCreate).toHaveBeenCalledWith('target_all.`' + PARQUET + '`'); + }); + it('drag carries the quoted identifier (but the graph payload keeps raw names)', () => { + const app = withParquet(); + renderSchema(app); + const d = dragstart(tbRow(app)); + expect(d[IDENT_MIME]).toBe('target_all.`' + PARQUET + '`'); + expect(JSON.parse(d[SCHEMA_GRAPH_MIME])).toEqual({ kind: 'table', db: 'target_all', table: PARQUET }); + }); + it('a column with special chars is quoted on insert', () => { + const app = withParquet(); + app.state.schema[0].tables[0].columns = [{ name: 'odd col', type: 'String', comment: '' }]; + app.state.expandedTables.add('target_all.' + PARQUET); + renderSchema(app); + const colRow = [...app.dom.schemaList.querySelectorAll('.tree-row.small')] + .find((r) => r.querySelector('.label').textContent === 'odd col'); + expect(dragstart(colRow)[IDENT_MIME]).toBe('`odd col`'); + shiftClick(colRow); + expect(app.actions.insertAtCursor).toHaveBeenCalledWith('`odd col`::String'); + }); +}); + describe('renderSchema filter', () => { it('keeps matching tables and drops non-matching ones', () => { const app = withSchema(); From dc8fbd267def1457915fabb03223d008415b7573 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Thu, 25 Jun 2026 23:10:30 +0200 Subject: [PATCH 2/2] fix(schema): address code-review findings on identifier quoting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the PR #43 self-review: - schema-graph.js: table-focus `center` still used the dot-heuristic qualify(), so dragging a dotted-name table (…snappy.parquet) onto the pane produced an EMPTY graph (center never matched a rowId node). Use joinId; add a regression test. Also simplify joinId to always emit `db.name` (guarantees a dot so node() splits correctly; was a `db ? … : name` ternary that could yield a dotless, mis-splittable id). - explain-graph.js: schemaClick no longer re-splits the node id — dagreLayout now passes db/name through (it already forwarded kind), so the UI quotes the parts the core already computed (no duplicated split convention). - completions.js: add a test asserting db/table/column `insert` is backtick- quoted for non-bare names (the quoting was previously unverified — bare names pass through unchanged, so a revert wouldn't have failed any test). - schema.js: hoist quoteIdent(db.db) to one local (was recomputed 3×). 868 tests pass; per-file coverage gate holds. Known follow-ups (not in this PR): the editor autocomplete path still can't match/qualify non-bare names (completionContext word/parent detection is [A-Za-z0-9_]-only), and qualify() remains a dot-heuristic for MV-target / EXPLAIN-AST names — both can drop/garble for dotted tables but the primary tree/graph vectors are correct. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu --- src/core/dot-layout.js | 5 +++-- src/core/schema-graph.js | 12 ++++++++---- src/ui/explain-graph.js | 13 ++++++------- src/ui/schema.js | 7 ++++--- tests/unit/completions.test.js | 15 +++++++++++++++ tests/unit/explain-graph.test.js | 9 +++++---- tests/unit/schema-graph.test.js | 13 +++++++++++++ 7 files changed, 54 insertions(+), 20 deletions(-) diff --git a/src/core/dot-layout.js b/src/core/dot-layout.js index 63804d4..4616643 100644 --- a/src/core/dot-layout.js +++ b/src/core/dot-layout.js @@ -41,8 +41,9 @@ export function dagreLayout(dagre, graph) { const outNodes = nodes.map((n) => { const dn = g.node(n.id); - // `kind` (node) / `label` (edge) pass through for the schema graph's colouring. - return { id: n.id, label: n.label, kind: n.kind, x: dn.x - dn.width / 2, y: dn.y - dn.height / 2, w: dn.width, h: dn.height }; + // `kind`/`db`/`name` (node) and `label` (edge) pass through for the schema + // graph's colouring + click-to-SHOW-CREATE (so the UI need not re-split the id). + return { id: n.id, label: n.label, kind: n.kind, db: n.db, name: n.name, x: dn.x - dn.width / 2, y: dn.y - dn.height / 2, w: dn.width, h: dn.height }; }); const outEdges = edges.map((e) => ({ from: e.from, to: e.to, kind: e.kind, label: e.label, diff --git a/src/core/schema-graph.js b/src/core/schema-graph.js index cbfb6e0..68b3a23 100644 --- a/src/core/schema-graph.js +++ b/src/core/schema-graph.js @@ -77,9 +77,10 @@ export function parseEngineRef(engine, engineFull) { // drops an edge rather than mis-linking. const qualify = (db, name) => (name && name.includes('.') ? name : db + '.' + name); // A reference whose db is always supplied separately (dependencies_*, engine -// args) — join unconditionally so a dotted table name (`…snappy.parquet`) keeps -// its db prefix instead of being mistaken for an already-qualified ref. -const joinId = (db, name) => (db ? db + '.' + name : name); +// args, table-focus center) — join unconditionally so a dotted table name +// (`…snappy.parquet`) keeps its db prefix instead of being mistaken for an +// already-qualified ref. Always emits a dot (like rowId) so node() can split it. +const joinId = (db, name) => db + '.' + name; const rowId = (r) => r.database + '.' + r.name; /** @@ -193,7 +194,10 @@ export function buildSchemaGraph(rows, focus) { let outNodes = [...nodes.values()]; let outEdges = edges; if (focus && focus.kind === 'table') { - const center = qualify(focus.db, focus.table); + // focus.table is always a bare name (db is separate in the drag payload), so + // join unconditionally — a dotted table name (`…snappy.parquet`) must keep its + // db prefix to match the rowId-built node ids, or the 1-hop filter finds nothing. + const center = joinId(focus.db, focus.table); const keep = new Set([center]); for (const e of edges) { if (e.from === center) keep.add(e.to); if (e.to === center) keep.add(e.from); } outNodes = outNodes.filter((n) => keep.has(n.id)); diff --git a/src/ui/explain-graph.js b/src/ui/explain-graph.js index df61765..e44d3f1 100644 --- a/src/ui/explain-graph.js +++ b/src/ui/explain-graph.js @@ -9,7 +9,7 @@ import { h, s } from './dom.js'; import { Icon } from './icons.js'; import { parseDot } from '../core/dot.js'; import { dagreLayout } from '../core/dot-layout.js'; -import { quoteIdent, qualifyIdent } from '../core/format.js'; +import { qualifyIdent } from '../core/format.js'; import { fitBox, zoomBox, panBox, viewBoxStr } from '../core/panzoom.js'; const ZOOM_STEP = 1.2; // per wheel notch / button press @@ -196,14 +196,13 @@ export function openPipelineFullscreen(app, rawText) { } // Clicking an object runs SHOW CREATE for it, dropping the (formatted) DDL into -// the editor — the same action as a shift-click in the schema tree. The id is -// `db.name` (split on the FIRST dot — a table name may itself contain dots); each -// part is quoted so non-bare names (`…snappy.parquet`) stay valid SQL. External -// dictionary-source leaves have no DDL. +// the editor — the same action as a shift-click in the schema tree. The node +// carries `db`/`name` separately (from buildSchemaGraph via dagreLayout), so each +// part is quoted independently — non-bare names (`…snappy.parquet`) stay valid SQL +// without re-splitting the id. External dictionary-source leaves have no DDL. const schemaClick = (app) => (n) => { if (!n.id || n.id.startsWith('ext:')) return; - const dot = n.id.indexOf('.'); - app.actions.insertCreate(dot < 0 ? quoteIdent(n.id) : qualifyIdent(n.id.slice(0, dot), n.id.slice(dot + 1))); + app.actions.insertCreate(qualifyIdent(n.db, n.name)); }; /** Fullscreen schema-lineage graph. */ diff --git a/src/ui/schema.js b/src/ui/schema.js index c29f127..ee31ef4 100644 --- a/src/ui/schema.js +++ b/src/ui/schema.js @@ -74,16 +74,17 @@ export function renderSchema(app) { const matches = (s) => !filter || s.toLowerCase().includes(filter); for (const db of state.schema) { + const qdb = quoteIdent(db.db); // SQL-safe db name (reused by the 3 emit sites) list.appendChild(h('div', { class: 'tree-row bold', title: 'Click to expand · double-click to insert · shift-click for SHOW CREATE', onclick: (e) => { - if (e.shiftKey) { app.actions.insertCreate('DATABASE ' + quoteIdent(db.db)); return; } - if (isDoubleClick(app, 'db:' + db.db)) { app.actions.insertAtCursor(quoteIdent(db.db)); return; } + if (e.shiftKey) { app.actions.insertCreate('DATABASE ' + qdb); return; } + if (isDoubleClick(app, 'db:' + db.db)) { app.actions.insertAtCursor(qdb); return; } db.expanded = !db.expanded; renderSchema(app); }, - ...lineageDrag(quoteIdent(db.db), { kind: 'db', db: db.db }), + ...lineageDrag(qdb, { kind: 'db', db: db.db }), }, ...treeRow(Icon.database(), db.db, String(db.tables.length), { expanded: db.expanded }), )); diff --git a/tests/unit/completions.test.js b/tests/unit/completions.test.js index 1ccab32..0933b59 100644 --- a/tests/unit/completions.test.js +++ b/tests/unit/completions.test.js @@ -91,6 +91,21 @@ describe('buildCompletions', () => { expect(items.some((i) => i.label === 'pending')).toBe(true); // table listed expect(items.some((i) => i.kind === 'column' && i.parent === 'pending')).toBe(false); // no columns expect(items.find((i) => i.label === 'empty')).toMatchObject({ kind: 'db' }); + // bare names insert verbatim (label === insert) + expect(items.find((i) => i.label === 'ontime')).toMatchObject({ insert: 'ontime' }); + }); + + it('backtick-quotes the insert for non-bare db/table/column names (label stays bare)', () => { + const schema = [{ + db: 'target_all', + tables: [{ name: 'part-0.snappy.parquet', columns: [{ name: 'odd col', type: 'String' }] }], + }]; + const items = buildCompletions(ref, schema); + expect(items.find((i) => i.label === 'target_all')).toMatchObject({ kind: 'db', insert: 'target_all' }); + expect(items.find((i) => i.label === 'part-0.snappy.parquet')) + .toMatchObject({ kind: 'table', insert: '`part-0.snappy.parquet`' }); + expect(items.find((i) => i.label === 'odd col')) + .toMatchObject({ kind: 'column', insert: '`odd col`' }); }); it('handles a null schema', () => { const items = buildCompletions(ref, null); diff --git a/tests/unit/explain-graph.test.js b/tests/unit/explain-graph.test.js index 4ac501f..d70b11c 100644 --- a/tests/unit/explain-graph.test.js +++ b/tests/unit/explain-graph.test.js @@ -165,10 +165,11 @@ describe('schema lineage graph', () => { afterEach(() => { document.body.innerHTML = ''; }); const GRAPH = { focus: { kind: 'db', db: 'lin' }, + // nodes carry db/name separately, as buildSchemaGraph produces them nodes: [ - { id: 'lin.a', label: 'a', kind: 'table' }, - { id: 'lin.mv', label: 'mv', kind: 'mv' }, - { id: 'lin.dst', label: 'dst', kind: 'table' }, + { id: 'lin.a', label: 'a', kind: 'table', db: 'lin', name: 'a' }, + { id: 'lin.mv', label: 'mv', kind: 'mv', db: 'lin', name: 'mv' }, + { id: 'lin.dst', label: 'dst', kind: 'table', db: 'lin', name: 'dst' }, ], edges: [ { from: 'lin.a', to: 'lin.mv', kind: 'feeds' }, @@ -196,7 +197,7 @@ describe('schema lineage graph', () => { it('clicking a node with a non-bare name backtick-quotes the SHOW CREATE target', () => { const actions = { insertCreate: vi.fn() }; - const g = { focus: { kind: 'db', db: 'target_all' }, nodes: [{ id: 'target_all.a-b.parquet', label: 'a-b.parquet', kind: 'table' }], edges: [] }; + const g = { focus: { kind: 'db', db: 'target_all' }, nodes: [{ id: 'target_all.a-b.parquet', label: 'a-b.parquet', kind: 'table', db: 'target_all', name: 'a-b.parquet' }], edges: [] }; const el = renderSchemaGraph({ document, Dagre: dagre, actions }, { schemaGraph: g }); el.querySelector('rect.eg-node--table').dispatchEvent(new Event('click', { bubbles: true })); expect(actions.insertCreate).toHaveBeenCalledWith('target_all.`a-b.parquet`'); diff --git a/tests/unit/schema-graph.test.js b/tests/unit/schema-graph.test.js index 0db051a..c83d396 100644 --- a/tests/unit/schema-graph.test.js +++ b/tests/unit/schema-graph.test.js @@ -195,6 +195,19 @@ describe('buildSchemaGraph', () => { expect(() => buildSchemaGraph(rows, { kind: 'db', db: 'lin' })).not.toThrow(); }); + it('table-focus on a dotted-name table keeps the center + its 1-hop (not empty)', () => { + const rows = { tables: [ + T('target_all', 'part-0.snappy.parquet', 'MergeTree', { dependencies_database: ['target_all'], dependencies_table: ['v_over_parquet'] }), + T('target_all', 'v_over_parquet', 'View'), + T('target_all', 'unrelated', 'MergeTree'), + ], dictionaries: [] }; + const g = buildSchemaGraph(rows, { kind: 'table', db: 'target_all', table: 'part-0.snappy.parquet' }); + const ids = new Set(g.nodes.map((n) => n.id)); + expect(ids.has('target_all.part-0.snappy.parquet')).toBe(true); // center kept its db prefix + expect(ids.has('target_all.v_over_parquet')).toBe(true); // 1-hop neighbour + expect(ids.has('target_all.unrelated')).toBe(false); + }); + it('keeps the db prefix for a dependency whose table name contains dots', () => { // dependencies_* carry the db separately, so a dotted table name (a parquet // file table) must still join to db., not be mistaken for db-qualified.