Repository navigation
feat(explorer): inspect a column's relationships and usage evidence - #1479
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| while (['LATERAL', 'ONLY', 'TABLE'].includes(keyword(tokens[i]))) i++; | ||
| let binding: Binding; | ||
| let defaultAlias: string | undefined; | ||
| if (tokens[i]?.value === '(') { |
There was a problem hiding this comment.
WARNING: First table inside a parenthesised join is never registered
When FROM is followed by (, readRelation treats it as a derived table: it pushes a ref-less relation and returns without reading anything inside the parens. The main loop then only picks up tables after a JOIN/, keyword inside, so the leading table is dropped.
MySQL's SHOW CREATE VIEW writes joins exactly this way, e.g. (dialect mysql, target orders.customer_id):
select `o`.`customer_id` from (`orders` `o` join `customers` `c` on((`c`.`id` = `o`.`customer_id`)))orders o is never bound, so o.customer_id is silently dropped (false negative) on the main view-definition use case. With 3+ tables MySQL nests ((a join b) join c), so the first table of every nest is lost. Tests only cover single-table MySQL views.
| // character at a time. | ||
| if ( | ||
| WORD_START.test(ch) || | ||
| (DIGIT.test(ch) && !WORD_CHAR.test(source[position - 1] ?? '')) |
There was a problem hiding this comment.
WARNING: Numeric literal after $ is not redacted (privacy leak)
WORD_CHAR includes $, so a digit preceded by $ skips the number branch and each digit is pushed as raw punct (lines 176-181). In dialects without dollar quoting this survives redaction. E.g. dialect mssql: SELECT customer_id FROM orders WHERE total > $100.50 yields a snippet/summary still containing $100.50 (MSSQL money literal); the same happens for $4111111111111111 in MySQL/SQLite/generic. The PR promises that every number literal is replaced with ?.
| const prev = tokens[i - 1]; | ||
| const bare = | ||
| keyword(prev) === 'SELECT' || keyword(prev) === 'DISTINCT' || prev?.value === ','; | ||
| if (bare && targetInStatement) { |
There was a problem hiding this comment.
WARNING: Wildcard check ignores which query level the * belongs to
targetInStatement is statement-wide, so a * in an unrelated scope is reported as uncertain:wildcard for the target. E.g. SELECT * FROM customers WHERE id IN (SELECT id FROM orders) or SELECT * FROM customers WHERE EXISTS (SELECT 1 FROM orders o WHERE o.cid = customers.id) flags orders.customer_id, although the * only expands customers. SELECT * inside an EXISTS (...) on the target never reads the column either. Please resolve the * against the tables of its own scope (as done for unqualified columns).
Also, line 606-607 only accepts SELECT/DISTINCT/, before *, so SELECT TOP 10 * FROM orders, SELECT ALL * and SELECT SQL_CALC_FOUND_ROWS * are missed.
| } | ||
| const word = keyword(token); | ||
| if (!word) continue; | ||
| if (LIST_END.has(word)) listDepths.delete(depth); |
There was a problem hiding this comment.
SUGGESTION: Comma join after an ON clause is not read as a relation
ON/USING are in LIST_END, which deletes listDepths for the current depth, so in SELECT customer_id FROM orders o JOIN x ON x.a = o.a, customers c the relation customers c is never registered. The unqualified customer_id is then reported parsed instead of uncertain:several-tables (a false parsed, which the PR says should not happen for ambiguous references).
| if (word === 'FROM' || word === 'UPDATE') listDepths.add(depth); | ||
| const next = readRelation(i + 1, word === 'FROM' || word === 'JOIN'); | ||
| if (next > i + 1) i = next - 1; | ||
| } else if (word === 'USING' && tokens[i + 1]?.value !== '(') { |
There was a problem hiding this comment.
SUGGESTION: USING without ( is always read as a relation
PL/pgSQL EXECUTE format(...) USING some_var registers some_var as a table, which makes model.relations.length non-zero and suppresses the textual fallback. The existing test only passes because USING 4 is a number token. Consider restricting this branch to statements that started with DELETE.
| // Table function: its columns are not the table's. | ||
| i = skipParens(tokens, i) + 1; | ||
| binding = {scope: scopes[start]}; | ||
| } else if (!schema && cteNames.has(name.toLowerCase())) { |
There was a problem hiding this comment.
SUGGESTION: CTE names are statement-global, so a CTE that reads the same-named table hides the real table
WITH orders AS (SELECT * FROM orders WHERE ...) SELECT customer_id FROM orders: the inner unqualified orders matches cteNames and is bound without a ref, so the real table reference is missed. Schema-qualified inner references are fine.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental changes (
I read the diffs of Files Reviewed (3 files)
Previous Review Summaries (5 snapshots, latest commit 3b025ec)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3b025ec)Status: No Issues Found | Recommendation: Merge The incremental changes (
I read the diffs of Files Reviewed (2 files)
Previous review (commit 7ad25cd)Status: No Issues Found | Recommendation: Merge The incremental changes (
The rest of the diff is merged Files Reviewed (1 file)
Previous review (commit cd88ffc)Status: No Issues Found | Recommendation: Merge The incremental changes (
New tests cover the collector changes. Files Reviewed (4 files)
Previous review (commit 7c4323a)Status: No Issues Found | Recommendation: Merge The incremental changes (
New tests in Files Reviewed (2 files in the incremental diff)
Previous review (commit e6c0116)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (19 files)
Reviewed by claude-sonnet-5.5 · Input: 248 · Output: 1K · Cached: 143.7K Review guidance: REVIEW.md from base branch |
|
Addressed the six review findings in
Official frontend checks ( |
|
In reply to #1479 (comment): Fixed in 7c4323a: a parenthesis after FROM/JOIN that opens with a table (or another parenthesis) is now read as a join group, so its tables are bound. Covered by tests for the two- and three-table nested MySQL view bodies, a Postgres group, |
|
In reply to #1479 (comment): Fixed in 7c4323a: a bare |
|
In reply to #1479 (comment): Fixed in 7c4323a: a digit after |
|
In reply to #1479 (comment): Fixed in 7c4323a: a CTE name now hides a table only after its own body (or inside it with |
|
In reply to #1479 (comment): Fixed in 7c4323a: |
|
In reply to #1479 (comment): Fixed in 7c4323a: |
|
The red |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @shu-chen-dot-ca, this is a thoughtful feature and it covers all of #1407. Two things I especially liked. The redaction goes through the existing sqlSplitter dialect rules, so $ numbers, N'' / X'' / E'', dollar-quoted bodies and MySQL double-quoted strings all end up as '?', and comments are dropped from excerpts and the summary. And each source reports complete, partial, unsupported, permission denied, failed or cancelled, so missing metadata never shows up as "no evidence". That honesty about what was actually checked is exactly what this panel needs.
Approving. I'd pick up the first point soon, the rest are follow-ups:
- Stop can't interrupt the history pass. The loop in
columnUsageCollector.tsnever awaits:so the Stop click handler can't run until the loop is done, andfor (const entry of inScope.slice(0, limits.maxHistoryEntries)) { if (scanned % 200 === 0 && cancelled()) {
cancelled()never sees the abort. With a large history (lots of big scripts) the UI freezes for seconds and Stop does nothing. Yielding every N entries (await new Promise((r) => setTimeout(r, 0))) before checkingcancelled(), plus a test that aborts during the history scan, would fix it. The current cancellation test only aborts inside a lookup mock. ForeignKeyhas noref_schema, so an incoming FK likepublic.invoices → sales.orders(customer_id)matches onref_table === 'orders'and is listed as Confirmed forpublic.orders.customer_id. Outgoing FKs also set noobjectSchema, so Open can go to the wrong table. #1407 mentions duplicate table names across schemas; until FKs carry the referenced schema, marking these as uncertain or stating it in the boundary text would be safer.computeScopesonly opens a new scope for(SELECT/(WITH, so both sides of a UNION share one.SELECT customer_id FROM public.orders UNION SELECT customer_id FROM public.archivegives two "uncertain, several tables" matches. The wildcard path already splits on set operators, the same could apply here.collectColumnUsage(...).then(...)inColumnUsageModal.tsxhas no.catch. Lookup errors are handled inside the collector, but an unexpected parser throw would leave "Searching…" and Stop on screen forever.- The description says the shared
Modalprovides Escape and focus, butModalonly wiresuseEscapeKey. AddingautoFocuson Close or Stop, like the other modals do, would move keyboard focus into the dialog. describe('review findings', …)incolumnUsage.test.tsshould be folded into thefindColumnReferences/buildUsageSnippetblocks (.rules/testing.md), and a component test forColumnUsageModaland the newSidebarColumnItemmenu (which now opens whencanManageis false) would be welcome.- Failed history entries (
status: 'error') count as evidence with nothing in the UI marking them. Tagging them would help.
About CI: the red test job is a single RowEditorPanel.test.tsx case that this PR doesn't touch and that passes locally, so it looks like a flake. I've re-run the failed job. I'll try the feature in the app before merging.
|
Thanks @debba! Pushed bb64594:
I also took the three small ones:
I'll take 2, 3 and 7 in a follow-up PR, along with the component tests for
The official frontend checks pass: 558 files, 8930 tests. |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for adding the history-scan yield, the unexpected-error settlement and initial focus. I checked cd88ffc3 against the reviewed revision: Stop can now run between batches, the collector rejection no longer leaves pending sources searching forever, and the tests were moved into the normal feature groups. The affected frontend suites, typecheck, lint and production build pass.
The cross-schema FK problem from Debba's review is still present, and #1474 now provides the referenced-schema metadata on main. I reproduced the false positive with an independent collector test:
- Inspect
public.orders.customer_id. - Return a snapshot containing
public.invoices.customer_id -> sales.orders.customer_id, withref_schema: "sales"on that FK. - Expect no incoming FK for
public.orders.
The collector instead emits an incoming item with confidence: "confirmed" and objectSchema: "public". It compares only ref_table and ref_column. The outgoing path also omits the referenced schema when building its navigation target.
Please integrate current main and use the referenced-schema identity in both FK directions, including the Open target. Add same-table-name/different-schema regressions so the panel cannot confidently report or navigate to the wrong relation. The independent test failed on this head and was removed afterward; the worktree is clean. I have not run a native column-usage flow in this pass.
Requesting changes for that remaining metadata-identity issue. The yield and error-handling corrections are good.
…dact $-prefixed numbers in column usage
|
Thanks @NewtTheWolf. Rebased onto current main (5199006, which includes #1474) with no conflicts, and added 2868b79:
New Official frontend checks ( |
cd88ffc to
7ad25cd
Compare
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for adding the referenced-schema checks and navigation target. The original public.invoices -> sales.orders regression now passes on 7ad25cd9, and the 63 affected collector/parser tests, typecheck, lint and production build pass.
I found a remaining catalog-identity case in the same path. For a PostgreSQL target public.orders.customer_id, I tested three valid distinct references: "PUBLIC".orders.customer_id, public."Orders".customer_id, and public.orders."CUSTOMER_ID". The incoming matcher emits a confirmed item for each because sameName case-folds the catalog names. These are distinct identifiers under PostgreSQL's quoted-identifier rules.
An independent metadata fixture with ordinary columns and both table names runs four cases: the original different-schema case passes, while all three quoted-name cases fail the assertion that no incoming item should be confirmed. The temporary probe was removed afterward; the worktree is clean. This was a collector test, not a native database session.
Please apply the connection dialect's identifier-identity rules to the FK schema/table/column comparisons and add these quoted PostgreSQL regressions. The getReferencedSchema fallback and outgoing schema propagation are good; I am keeping changes requested for the remaining false confirmations.
|
Thanks @NewtTheWolf, good catch. Pushed a9c4846:
Tests cover:
The new test groups fail on 7ad25cd. Not changed: aliases are still matched without case, so in PostgreSQL Official frontend checks ( The red |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for separating catalog names from SQL spelling. On 3b025ecd, all four independent PostgreSQL regressions now pass, and the 67 affected tests, typecheck, lint and production build pass too.
The new default rule still produces false confirmed FKs for two other supported cases:
- SQLite non-ASCII identifiers. I created both
"Ä"and"ä"in a real in-memory SQLite database, withinvoices.parent_idreferencing"Ä".id. SQLite accepts the two tables as distinct. Feeding its actual table/foreign-key metadata to the collector while inspecting"ä".idproduces an incoming confirmed item.identifierRulesFor('sqlite')usesinsensitive, and JavaScripttoLowerCase()conflates those names. SQLite's identifier folding does not make that Unicode pair equivalent. - SQL Server case-sensitive collation. The same metadata fixture with
dbo.Ordersanddbo.ordersproduces a confirmed incoming key for the wrong table. SQL Server identifier identity depends on collation, so the default cannot always assume case-insensitive names; see Microsoft's identifier documentation. This second case was a collector fixture, not a live SQL Server session.
The independent probe has a positive control for the real FK target in each dialect: both controls pass, and both wrong-target assertions fail. The SQLite metadata and probe are saved in the local evidence; the temporary test file was removed.
Please make SQLite folding ASCII-aware and handle SQL Server catalog identity conservatively when collation is unknown, with regressions for both cases. PostgreSQL is corrected; I am keeping changes requested because the current rules can still label a reference to another object as confirmed.
|
Thanks @NewtTheWolf. Pushed 9893f53:
Tests:
The three new test groups fail on 3b025ec. I checked the SQLite behaviour with Official frontend checks ( |
There was a problem hiding this comment.
Thanks for working through the identifier cases as well as the history cancellation. The collector now uses the referenced schema in both FK directions, PostgreSQL catalog names compare exactly, SQLite folds only ASCII letters, and SQL Server stays conservative when the collation is unknown. Those changes avoid confidently pointing at a different object. The history loop also yields before checking cancellation, and unexpected collector errors settle the pending sources.
All 69 parser/collector tests and frontend typecheck pass on this revision, including the SQLite and SQL Server regressions.
Approving, with two follow-ups:
- History evidence still drops the entry's
statusanderror, so a failed query is shown without identifying it as a failed attempt. Please carry that distinction into the evidence and its display. - The tests remain utility-level. A component test for initial focus, Stop, unexpected failure and opening a referenced object would complement them well.
I have now tested this head's frontend and backend in T3's integrated browser against real PostgreSQL and SQLite fixtures. Cross-schema FK filtering and Open navigation work, the view reference appears, Stop interrupts a 2,000-script history scan after only part of it is processed, and a deliberately failed view lookup settles and recovers on retry. SQLite Ä.id and ä.id remain distinct, with the positive FK only on Ä.id. Initial focus and Escape also work. Proceeding with the merge; the follow-ups above remain non-blocking.
Fixes #1407.
Adds a Show Usage action to a column's context menu in the explorer sidebar. It opens a panel that collects, for that one column, the declared foreign keys, references in view definitions and matches in this connection's Tabularis query history. Every item shows its source, an excerpt and a confidence level, instead of implying complete lineage. The boundary (database metadata plus local history) is stated in the panel.
What changed
utils/columnUsage.ts(pure): a dialect-aware reference finder built on the existingsqlSplittertokenizer (optionsFor(dialect), plus pluginsql_syntaxthroughresolveSqlSyntax), so quoting, comments, dollar quotes and MySQL/*! */follow the connection's dialect.SELECT/WITHparenthesis is its own scope. Aliases, CTEs, derived tables and table functions are bindings there, and unqualified columns also resolve against enclosing scopes (correlated subqueries).identifierRulesFor):"Ä"and"ä"stay distinct.schema.table, either qualified or through an alias.*, dynamic SQL (EXECUTE,PREPARE,sp_executesql), a name that differs only by case where that depends on server settings (MySQL table names withlower_case_table_names, SQL Server and unknown dialects with the collation), or a textual match only.'?'/?(includingX'',0x..,b'',N'',E'', dollar-quoted bodies and MySQL double-quoted strings).--,#and/* */comments are dropped from excerpts and the summary, since they often carry ids, emails or notes. The summary never includes raw SQL.utils/columnUsageCollector.ts: runs the lookups (get_foreign_keys,get_schema_snapshot,get_views/get_view_definition,get_query_history) one after the other. All of these already exist on both desktop and web transports, so there are no backend changes.AbortSignal. The history scan yields to the event loop, so Stop works during it.getReferencedSchema:ref_schemafrom fix(postgres): resolve cross-schema foreign key references #1474, else the source schema), table and column, in both directions, using the same identifier rules. Incoming keys come from the target schema's snapshot, and outgoing keys open in the schema they reference.ColumnUsageModal: shows the scope (connection / database / schema / table / column), the per-source status and the Confirmed / Parsed / Uncertain groups with their reasons. Open goes to the related table or view throughuseDatabaseObjectNavigation. Open in editor loads a history entry into a console without running it. Stop, Search again and Copy summary are also there. It is built on the existingModal(Escape), puts initial focus on Close, uses design tokens and has visible focus rings.SidebarColumnItem: the context menu now opens for every table column. Modify and Delete are still shown only where the column can be managed.Validation
tests/utils/columnUsage.test.tsandtests/utils/columnUsageCollector.test.ts. They cover:"PUBLIC","Orders","CUSTOMER_ID"), Oracle folding, MySQL table names in another case, SQLite"Ä"/"ä"(with theREFERENCES ORDERS(ID)spelling SQLite reports), and SQL Serverdbo.Orders/dbo.orderswith unknown collation;pnpm run typecheck && pnpm run lint && pnpm run web:boundaries && pnpm run web:manual-parity && pnpm exec vitest --config packages/web-ui/vitest.config.ts run --maxWorkers=2. All passed: 561 files and 9009 tests.ExplainGraph's 50 msfitViewtimer firing after jsdom teardown inVisualExplainView.test.tsx, which is unrelated to this change; that file passes on its own.diston the nightly binary of the first version's base commit) against PostgreSQL 16, withpublic.ordersandsales.orders, a composite FK and three views:public.orders.customer_idshows 2 confirmed FKs, 1 parsed view, 3 parsed and 1 uncertain history queries;sales.ordersqueries and unrelated views are excluded;impact SidebarColumnItem(upstream) gives LOW risk, with 2 direct callers and 1 flow, all inExplorerSidebar.impactofcollectColumnUsageandfindColumnReferences(upstream) gives LOW risk. Everything else is new files and locale entries.Screenshots