Skip to content

fix(sqllab): quote autocomplete table names that need it#41199

Open
jesperct wants to merge 1 commit into
apache:masterfrom
jesperct:fix/sqllab-autocomplete-quote-table-name
Open

fix(sqllab): quote autocomplete table names that need it#41199
jesperct wants to merge 1 commit into
apache:masterfrom
jesperct:fix/sqllab-autocomplete-quote-table-name

Conversation

@jesperct

@jesperct jesperct commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

SQL Lab autocomplete inserted table names exactly as stored. When a name isn't a simple identifier (it contains spaces or punctuation, or starts with a digit), inserting it verbatim produces invalid SQL. Picking a table like COVID Vaccines from the dropdown gave SELECT * FROM COVID Vaccines, which fails to parse.

This quotes the inserted value with double quotes when the name isn't a simple identifier, doubling any embedded quotes so names containing " stay valid. Simple identifiers are still inserted as-is, and the human-readable label shown in the dropdown is unchanged.

Quoting uses ANSI double-quote identifiers. Dialect-specific quote characters (e.g. backticks on MySQL) would be a reasonable follow-up.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
sc102556-before

AFTER:
sc102556-after

TESTING INSTRUCTIONS

  1. In SQL Lab, connect to a database/schema that has a table whose name needs quoting (for example, a name containing a space such as COVID Vaccines).
  2. Start typing the table name in the editor and pick it from the autocomplete dropdown.
    • Before: the name is inserted unquoted (FROM COVID Vaccines) and the query fails to parse.
    • After: the name is inserted quoted (FROM "COVID Vaccines") and runs.
  3. Confirm a normal table name (e.g. simple_table) is still inserted without quotes.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
  • Introduces new feature or API
  • Removes existing feature or API

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.34%. Comparing base (7340d06) to head (0522ac1).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #41199   +/-   ##
=======================================
  Coverage   64.34%   64.34%           
=======================================
  Files        2653     2653           
  Lines      144963   144968    +5     
  Branches    33452    33454    +2     
=======================================
+ Hits        93281    93286    +5     
  Misses      49997    49997           
  Partials     1685     1685           
Flag Coverage Δ
javascript 68.56% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jesperct jesperct marked this pull request as ready for review June 18, 2026 13:44
@dosubot dosubot Bot added the sqllab Namespace | Anything related to the SQL Lab label Jun 18, 2026
@bito-code-review

bito-code-review Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #f1a006

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 8c307be..8c307be
    • superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.test.ts
    • superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

SQL Lab autocomplete inserted table names verbatim, so picking a name
with spaces or other non-identifier characters (e.g. "COVID Vaccines")
produced invalid SQL. Quote the inserted value with double quotes (and
double any embedded quotes) when the name isn't a simple identifier;
simple names are still inserted as-is. The display label is unchanged.
@jesperct jesperct force-pushed the fix/sqllab-autocomplete-quote-table-name branch from 8c307be to 0522ac1 Compare June 22, 2026 14:12
allCachedTables.map(({ value, label, schema: tableSchema }) => ({
name: label,
value,
value: quoteIdentifier(value),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Using the quoted identifier as the table completion value breaks downstream table metadata flows because this value is also passed to addTable on selection. That stores names like "COVID Vaccines" (with quotes) in pinned table state, and later metadata requests use that quoted string as the name query param, which can fail table lookups that expect the raw table name. Keep autocomplete insertion quoting separate from the canonical table identity used by state/API calls. [api mismatch]

Severity Level: Critical 🚨
- ❌ SQL Lab table preview fails for quoted-name tables.
- ❌ Table metadata panel shows missing columns for those tables.
- ⚠️ Column autocomplete misses columns from pinned quoted tables.
Steps of Reproduction ✅
1. Open SQL Lab editor, which renders `EditorWrapper` at
`superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx:104-120` and passes
`keywords={keywords}` from `useKeywords(...)` at lines 26-35 and 382.

2. `useKeywords` builds `tableKeywords` in
`superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts:204-210`, setting
each table keyword to `{ name: label, value: quoteIdentifier(value), schema: tableSchema,
meta: 'table', completer: { insertMatch } }`, so a table like `COVID Vaccines` produces
`value: "\"COVID Vaccines\""`.

3. In the editor, trigger autocomplete and select the `COVID Vaccines` table completion;
Ace calls `insertMatch` defined in `useKeywords.ts:165-187`, which for `data.meta ===
'table'` dispatches `addTable(..., data.value, catalog ?? null, data.schema ?? schema ??
'', false)` using the quoted `data.value` (`"COVID Vaccines"`) as the `tableName`.

4. `addTable` in `superset-frontend/src/SqlLab/actions/sqlLab.ts:22-37` stores this quoted
`tableName` as `table.name`, and later `SouthPane` at
`superset-frontend/src/SqlLab/components/SouthPane/index.tsx:82-103` passes
`tableName={name}` into `TablePreview`, which uses `tableName` as the `table` parameter to
`useTableMetadataQuery` (`TablePreview/index.tsx:136-143`), leading `tables.ts:14-19` to
call `/api/v1/database/{dbId}/table_metadata/?name="%22COVID%20Vaccines%22"&schema=...`;
because the backend expects the raw table name (`COVID Vaccines`) without quotes, metadata
lookups and previews for tables pinned via autocomplete can fail.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts
**Line:** 208:208
**Comment:**
	*Api Mismatch: Using the quoted identifier as the table completion `value` breaks downstream table metadata flows because this `value` is also passed to `addTable` on selection. That stores names like `"COVID Vaccines"` (with quotes) in pinned table state, and later metadata requests use that quoted string as the `name` query param, which can fail table lookups that expect the raw table name. Keep autocomplete insertion quoting separate from the canonical table identity used by state/API calls.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not apply on the Ace editor path. toAceKeyword in AceEditorProvider.tsx copies only name/value/score/meta/docText/docHTML and drops completer, so insertMatch never runs and addTable is not called on selection. Ace inserts value directly, which is why the fix quotes value. No quoted name reaches table state or the metadata API through this path. The addTable side effect on select is already inactive for the same reason, independent of this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customized review instruction saved!

Instruction:

Do not flag quoted table completion values in the Ace editor path when toAceKeyword strips completer; in that flow insertMatch does not run, so autocomplete insertion does not call addTable or store quoted names in table state/API calls.

Applied to:

  • superset-frontend/src/SqlLab/components/AceEditorProvider.tsx
  • superset-frontend/src/SqlLab/components/EditorWrapper/**

💡 To manage or update this instruction, visit: CodeAnt AI Settings

@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. The current implementation uses the quoted identifier as the value property for autocomplete, which is then passed directly to addTable upon selection. This causes the application to store the quoted string as the canonical table name, leading to failures in downstream API calls that expect the raw table name.

To resolve this, you should separate the value used for editor insertion (which requires quoting) from the value used for state/API identification (which should be the raw name). You can update the completer object in useKeywords.ts to handle the insertion logic separately, ensuring addTable receives the raw value while the editor still receives the quoted string for correct SQL syntax.

Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?

superset-frontend/src/SqlLab/components/EditorWrapper/useKeywords.ts

name: label,
        value: value, // Use raw value for state/API
        schema: tableSchema,
        score: TABLE_AUTOCOMPLETE_SCORE,
        meta: 'table',
        completer: {
          insertMatch: (editor: any, data: any) => {
            editor.insert(quoteIdentifier(value)); // Insert quoted value into editor
            addTable(..., value, ...); // Use raw value for state
          }
        }

@bito-code-review

Copy link
Copy Markdown
Contributor

Bito Automatic Review Failed - Technical Failure

Bito encountered technical difficulties while generating code feedback . To retry, type /review in a comment and save. If the issue persists, contact support@bito.ai and provide the following details:

Agent Run ID: 8d2afd9c-682b-46a7-98f5-afb4fa569347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M sqllab Namespace | Anything related to the SQL Lab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant