fix(autocomplete): suggest clause keywords and correct columns after …#295
fix(autocomplete): suggest clause keywords and correct columns after …#295thomaswasle wants to merge 2 commits into
Conversation
| 'except', 'select', 'from', 'into', 'values', 'update', 'delete', 'insert', | ||
| 'create', 'drop', 'alter', 'and', 'or', 'not', 'is', 'in', 'between', | ||
| 'like', 'as', 'distinct', 'case', 'when', 'then', 'else', 'end', 'by', | ||
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', |
There was a problem hiding this comment.
CRITICAL: Missing 'using' in SQL_RESERVED set
USING is a standard SQL keyword in JOIN ... USING(...) clauses. The fromPattern regex captures the word after a table name as a potential alias, so FROM a JOIN b USING(id) incorrectly registers using as the alias for b. This breaks dot-trigger autocomplete for that table.
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', | |
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', 'using', |
|
|
||
| if (actualTableName) { | ||
| const columns = await getTableColumns(connectionId, actualTableName, schema); | ||
| const foundTable = tables.find(t => t.name.toLowerCase() === typedName); |
There was a problem hiding this comment.
WARNING: Alias dot-trigger uses wrong schema lookup in multiDb mode
When typedName is an alias (e.g. u aliasing users), this looks up the table list by the alias instead of the resolved actualTableName. In multiDb mode foundTable will be undefined, so schema falls back to activeSchema (which is null), and MySQL receives no schema filter.
The context-aware path already looks up by actual table name — the dot-trigger path should do the same.
| const foundTable = tables.find(t => t.name.toLowerCase() === typedName); | |
| const foundTable = tables.find(t => t.name.toLowerCase() === actualTableName.toLowerCase()); |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previous issues have been resolved in the latest commit. Resolved Issues (2)
Files Reviewed (5 files)
Reviewed by kimi-k2.6-20260420 · 148,642 tokens |
…WHERE
- Remove the `contextColumnSuggestions.length === 0` guard on keyword
suggestions so WHERE, ORDER BY, GROUP BY, LIMIT etc. are always offered
alongside column/table completions when a FROM clause is present
- Add SQL_RESERVED set to parseTablesFromQuery so keywords like WHERE/ON/
HAVING are never mistaken for table aliases, which was corrupting the
alias map and blocking column lookups
- Tag each TableInfo with its source database in multiDb mode (Editor.tsx)
and use t.schema ?? schema in getTableColumns calls so MySQL multiDb
connections pass the correct schema rather than falling back to
information_schema
- Fall back to a synthetic {name} entry when the loaded tables list is
empty so column fetches are still attempted before the first tables
payload arrives
- Skip caching empty column results to avoid poisoning the cache with
transient empty responses caused by schema mismatches or a not-yet-ready
connection
78ac1e6 to
c87419e
Compare
|
uhhh thx @thomaswasle i will take a look today! @debba u can assign it to me |
NewtTheWolf
left a comment
There was a problem hiding this comment.
Tested this end-to-end on the demo stack and all four fixes work: in MySQL multiDb the columns now resolve to the real table instead of information_schema, clause keywords show up after FROM, WHERE is no longer captured as an alias, and partial-filtering + the dot-trigger both behave. No regression on Postgres. Nice fix 👍
One small thing I'd want completed before merge, plus a few non-blocking notes.
Requesting changes
SQL_RESERVEDis incomplete — it covers the common clause keywords but misses a few that can legally follow a table name, so they're still captured as aliases (the exact bug this set is meant to prevent):SELECT * FROM orders FOR UPDATE→forbecomes an alias oforders;FROM t NATURAL JOIN u→natural. One-line addition — inline suggestion below.
Non-blocking (happy to spin any of these into separate issues instead of holding the PR):
- Denylist vs. parse depth —
SQL_RESERVEDpatches a greedy regex that grabs the first word after the table as the alias. A negative-lookahead / clause-boundary parse would make the denylist unnecessary (no perpetual keyword maintenance). The completion above fixes the immediate case; this is the longer-term shape. - multiDb same-name tables —
tables.find(t => t.name === …)returns the first match, so if the same table name exists in two selected DBs the first DB's schema always wins, anddb.table.qualifiers aren't parsed. Still strictly better than before (which returned no columns at all) — worth a follow-up for cross-DB disambiguation. - Empty-result cache skip — deliberate per your comment (see inline); just one rare edge case to consider.
- Keyword clutter / duplicate labels — keywords now always render, so a column named like a keyword (
count,name,value) shows twice (Field + Keyword). Relies entirely onsortTextordering. UX tuning. - Three keyword lists —
SQL_RESERVEDoverlapsSQL_KEYWORDS(autocomplete.ts) and the sets insqlSplitter/classify.ts; a shared source would avoid drift.
Inline details below.
| 'except', 'select', 'from', 'into', 'values', 'update', 'delete', 'insert', | ||
| 'create', 'drop', 'alter', 'and', 'or', 'not', 'is', 'in', 'between', | ||
| 'like', 'as', 'distinct', 'case', 'when', 'then', 'else', 'end', 'by', | ||
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', 'using', |
There was a problem hiding this comment.
Blocking (cheap): this set misses a few keywords that can legally follow a table name, so they're still captured as aliases — the exact bug it's meant to prevent:
SELECT * FROM orders FOR UPDATE→forregistered as alias ofordersFROM t NATURAL JOIN u→naturalas alias oft- also
FETCH(FETCH FIRST),WINDOW,LATERAL,STRAIGHT_JOIN(MySQL),TABLESAMPLE,QUALIFY
Adding them completes the fix:
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', 'using', | |
| 'asc', 'desc', 'null', 'with', 'exists', 'all', 'any', 'using', | |
| 'for', 'fetch', 'natural', 'window', 'lateral', 'straight_join', 'tablesample', 'qualify', |
(Longer term, a negative-lookahead in the alias capture would remove the need to maintain this list at all — but that's non-blocking.)
There was a problem hiding this comment.
@NewtTheWolf I decided to skip this and directly went for the long term solution.
| // Don't cache empty results — they likely reflect a transient failure | ||
| // (connection not ready, schema mismatch) rather than a genuinely empty table. | ||
| // An uncached miss retries the backend on the next trigger, which is cheap. | ||
| if (simpleCols.length > 0) { |
There was a problem hiding this comment.
Non-blocking — the rationale in your comment is sound (empty ≈ transient/not-ready, retry is cheap) and the cache already has a TTL on success, so skipping empties is a reasonable trade-off.
One edge case to keep in mind: a genuinely empty relation (a 0-column view, or a table the backend can't introspect) is never cached, so it re-issues get_columns on every trigger that references it. Almost certainly fine given how rare that is — but if it ever shows up as IPC chatter, distinguishing "empty because not-ready" from "empty because actually-empty" (or a short negative-cache TTL) would bound it. Fine to leave as a follow-up issue.
…e TTL
sqlAnalysis
- Replace SQL_RESERVED denylist with clause-boundary extraction: isolate
the FROM/JOIN section (stop at WHERE/GROUP BY/etc.) then strip ON/USING
conditions, so no clause keyword can ever land in the alias capture group
- Export ParsedTableRef {name, schema} and update parseTablesFromQuery to
return Map<string, ParsedTableRef>; regex now captures schema.table
qualified notation (db.table alias or FROM db.table t)
autocomplete — dot-trigger
- Try db.table. two-dot pattern first; resolve by exact (schema, name) pair
- For unqualified alias/name, resolve via alias map (which now carries schema
from qualified FROM references) then fall back to .filter across all loaded
tables so every selected DB is covered when table name is ambiguous
autocomplete — context-aware columns (WHERE etc.)
- Qualified refs (FROM db.users) → exact (schema, name) lookup, one DB only
- Unqualified refs (FROM users) → expand to all loaded tables with that name
across all selected databases, deduplicated by column label
autocomplete — keyword dedup
- Filter keyword suggestions against already-collected column labels so a
column named like a keyword (count, name, value) no longer shows twice
autocomplete — column cache
- Add per-entry ttl field to CachedColumns; successful empty responses are
now cached with a 30 s negative TTL instead of being skipped entirely,
bounding repeated get_columns IPC calls against empty tables/views;
errors remain uncached and retry on the next trigger
09e3d8b to
eaec661
Compare
closes #293.
Summary
Keywords never suggested after FROM clause — The completion provider only
offered keyword completions when no context columns were available. Removed the
contextColumnSuggestions.length === 0guard so clause keywords (WHERE, ORDER BY,GROUP BY, LIMIT, …) are always included alongside column and table suggestions.
SQL keywords captured as table aliases —
parseTablesFromQuerymatchedFROM users WHEREand registeredWHEREas the alias, poisoning the alias map. Added aSQL_RESERVEDset; any captured alias that is a reserved word is discarded and thetable name is used as its own alias.
Wrong columns in MySQL multiDb mode — When
activeSchemais null (multiDb),getTableColumnswas called without a schema, causing MySQL to fall back toinformation_schemaand return system columns instead of the user's table columns.Fixed by adding an optional
schema?: stringfield toTableInfoand populating itwith the source database name when building
effectiveTablesin multiDb mode. Boththe context-aware and dot-trigger paths now pass
t.schema ?? schema.Column cache poisoning — The fallback that attempts column fetches before the
tables list loads could receive an empty response (schema mismatch / connection not
yet ready) and cache it, blocking all subsequent fetches for that table. Empty results
are now never written to the cache.
Files changed
src/utils/autocomplete.tst.schemain context-aware path; skip caching empty resultssrc/utils/sqlAnalysis.tsSQL_RESERVEDset; filter captured aliases against itsrc/contexts/DatabaseContext.tsschema?: stringtoTableInfosrc/pages/Editor.tsxeffectiveTablesin multiDb modeTest plan
SELECT * FROM <table>→ columns from the correct databaseappear
SELECT * FROM <table> WHERE→ clause keywords (AND, OR,…) and column names both appear
SELECT * FROM <table> WHERE <partial>→ matching column names filtercorrectly
table.→ columns from the correct per-database table appear