You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/08/18 23:17:33 UTC
(superset) 01/01: fix(dataset): derive virtual dataset schema from SQL Lab query
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-sqllab-virtual-dataset-schema-mismatch
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a32225646cccbc52a15b94526f10bc2eb427477b
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Tue Aug 18 23:10:04 2026 +0000
fix(dataset): derive virtual dataset schema from SQL Lab query
Derive an unambiguous catalog and schema from the Jinja-aware parsed query while preserving submitted defaults for ambiguous, mutating, metadata, or unparseable SQL. Include template parameters and conservative guards for quoted identifiers and incomplete parser results.
Addresses apache/superset#16791.
---
RCA.md | 346 +++++++++++++++++++++++
superset/commands/dataset/create.py | 84 +++++-
superset/sql/parse.py | 33 +++
tests/unit_tests/commands/dataset/test_create.py | 180 ++++++++++++
4 files changed, 642 insertions(+), 1 deletion(-)
diff --git a/RCA.md b/RCA.md
new file mode 100644
index 00000000000..2f32165c985
--- /dev/null
+++ b/RCA.md
@@ -0,0 +1,346 @@
+# RCA — SQL Lab virtual dataset saves the dropdown schema, not the query's schema
+
+**Issue:** GitHub apache/superset#16791
+**Analyzed commit:** `34cd50cc4840d72636f5d33c196ba3e0fcb8a7db`
+**Scope:** where the virtual dataset's `schema` is populated, and how to make it reflect the query.
+Permissions/RLS enforcement is explicitly out of scope.
+
+Every causal claim below is labelled `verified` (read end-to-end, or executed) or
+`inferred` (reasoned, unconfirmed).
+
+---
+
+## What Happened
+
+In SQL Lab a user selects database `examples` and schema `information_schema` in the two
+left-hand dropdowns, then runs a query that qualifies its tables against a *different*
+schema:
+
+```sql
+select * from public."Vehicle Sales"
+```
+
+They click **EXPLORE**, name the dataset, and click **Save & Explore**, creating a new
+virtual dataset (`SqlaTable` with a non-empty `sql`).
+
+- **Expected:** the saved dataset's `schema` is `public` — the schema the query actually reads.
+- **Actual:** the saved dataset's `schema` is `information_schema` — the SQL Lab dropdown value.
+
+The POST body sent to `/api/v1/dataset/` is (`verified`, read from
+`superset-frontend/src/SqlLab/actions/sqlLab.ts:1660-1700`):
+
+```json
+{ "database": 1, "catalog": null, "schema": "information_schema",
+ "sql": "select * from public.\"Vehicle Sales\"", "table_name": "my dataset", ... }
+```
+
+Reported and independently reproduced since 2021; never fixed.
+
+---
+
+## Root Cause
+
+Single-causal. **`schema` is carried verbatim from the SQL Lab dropdown to the persisted
+column, and no code on that path ever looks at the SQL.** (`verified` — traced end-to-end.)
+
+The chain, in order:
+
+| # | Location | What it does |
+|---|---|---|
+| 1 | `superset-frontend/src/utils/datasourceUtils.ts:54` | `schema: source.schema \|\| null` — reads the QueryEditor/Query object, whose `schema` is the dropdown selection |
+| 2 | `superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:369` | `schema: datasource?.schema ?? ''` |
+| 3 | `superset-frontend/src/SqlLab/actions/sqlLab.ts:1660-1700` | `createDatasource` puts that `schema` straight in the POST body |
+| 4 | `superset/datasets/api.py:441-509` | `POST /api/v1/dataset/` → `CreateDatasetCommand(item).run()` |
+| 5 | `superset/datasets/schemas.py:172,174` | `DatasetPostSchema` accepts `schema` and `sql` as independent fields, no cross-field validation |
+| 6 | **`superset/commands/dataset/create.py:60`** | **`schema = self._properties.get("schema")` — the defect anchor. The value is read from the payload and never reconciled against `sql` (read on the very next line, 62).** |
+| 7 | `superset/daos/base.py:482` | `setattr(item, key, value)` writes `schema` onto the new `SqlaTable` |
+| 8 | `superset/connectors/sqla/models.py:1531` | `schema = Column(String(255))` — persisted |
+
+`create.py:97-104` **does** parse the SQL — but only to hand it to
+`security_manager.raise_for_access(...)`. The parse result is discarded; it is never used
+to inform the stored `schema` (`verified`).
+
+### The stored `schema` is a *default schema*, not a label
+
+The default-schema hypothesis is confirmed. `superset/db_engine_specs/base.py:1838-1840`
+states it outright (`verified`):
+
+> "This is used in SQL Lab, allowing users to select a schema from the list of schemas
+> available in a given database, and have the query run with that schema as the default one."
+
+Downstream, the dataset's `schema` is consumed as the fallback for *unqualified* table
+references, via `Table.qualify()` (`superset/sql/parse.py:414-427`), which fills in
+`self.schema or schema` — i.e. explicit schemas win, the stored value is only a default:
+
+- `superset/models/helpers.py:3147` — `apply_rls(..., self.schema or default_schema or "", ...)` resolves RLS against it (`verified`).
+- `superset/models/helpers.py:1949-1953` — `self.database.get_df(sql, self.catalog, self.schema, ...)` at chart execution (`verified`).
+- `superset/security/manager.py:4174-4180` — permission checks qualify parsed tables with it (`verified`).
+
+Whether it changes execution is **engine-dependent** (`verified`): Presto/Trino write it
+into the connection URI (`superset/db_engine_specs/presto.py:344-362`), so it really is
+the engine's default schema; Postgres ignores it
+(`superset/db_engine_specs/postgres.py:662-675` sets only the catalog). So in the
+Postgres repro the wrong value does not break the chart — it silently mis-scopes
+metadata, RLS resolution, and permission checks.
+
+---
+
+## Why It Wasn't Caught
+
+1. **The buggy behaviour is codified as a passing test** (`verified`).
+ `superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx:249-268`
+ is literally named `'sends the schema when creating the dataset'` and asserts
+ `schema: 'main'` is forwarded. It was written to lock in the passthrough as the spec,
+ so nobody read the passthrough as a defect.
+2. **No backend test asserts anything about `schema` on virtual-dataset creation**
+ (`verified`). `tests/unit_tests/commands/dataset/test_create.py` has zero occurrences
+ of the string `schema` outside imports; its virtual-dataset cases
+ (`test_create_dataset_invalid_sql_parse_error`, `..._valid_sql_with_access_error`)
+ assert only on error handling.
+3. **Assumption gap:** the dropdown schema and the query's schema coincide in the
+ overwhelmingly common case (users pick the schema they're about to query). The defect
+ only surfaces when a user deliberately cross-references, which no fixture does.
+4. **The parse result was right there and unused** (`verified`). Because `create.py:99`
+ already parses the SQL for the *security* question, a reviewer scanning the file sees
+ "this command does look at the SQL" and does not notice that the *schema* question was
+ never asked of it.
+
+---
+
+## The Fix
+
+**File:** `superset/commands/dataset/create.py`, inside `validate()`.
+**Insertion point:** after catalog defaulting (line 73), **before** `table = Table(...)`
+at line 75.
+
+**Before** (lines 60, 73-75):
+```python
+schema = self._properties.get("schema")
+...
+ if not catalog:
+ catalog = self._properties["catalog"] = database.get_default_catalog()
+
+ table = Table(table_name, schema, catalog)
+```
+
+**After** — when `sql` is present and the Jinja-aware parse unambiguously names one
+catalog/schema pair, replace the dropdown value (and write it back to `_properties`
+so the DAO persists it). The implementation logs and falls back for each unsafe case;
+the core decision is:
+```python
+ if not catalog:
+ catalog = self._properties["catalog"] = database.get_default_catalog()
+
+ if sql:
+ try:
+ template_params = json_to_dict(
+ self._properties.get("template_params") or ""
+ )
+ parse_result = process_jinja_sql(sql, database, template_params)
+ except Exception:
+ logger.debug(...)
+ else:
+ script = parse_result.script
+ tables = {table for table in parse_result.tables if table.table}
+ if (
+ not script.has_unparseable_statement
+ and not script.has_mutation()
+ and not script.changes_default_schema()
+ and not script.has_quoted_table_location()
+ and tables
+ and all(table.schema is not None for table in tables)
+ ):
+ qualified_pairs = {
+ (table.catalog, table.schema) for table in tables
+ }
+ if len(qualified_pairs) == 1:
+ derived_catalog, derived_schema = qualified_pairs.pop()
+ schema = self._properties["schema"] = derived_schema
+ if derived_catalog is not None:
+ catalog = self._properties["catalog"] = derived_catalog
+
+ table = Table(table_name, schema, catalog)
+```
+
+The new logic reuses the Jinja-aware parser already used by the access check — **no new
+SQL parsing is written**. `process_jinja_sql()` returns both the processed `SQLScript`
+and the union of ordinary SQL table references plus tables harvested from partition
+macros (`verified`; see the matrix below):
+
+```python
+parse_result = process_jinja_sql(sql, database)
+script = parse_result.script
+tables = parse_result.tables
+```
+
+### Adopt the query's schema only when ALL of these hold
+
+| # | Condition | Why (all `verified` by execution) |
+|---|---|---|
+| 1 | Optional template-parameter decoding and Jinja-aware processing succeed | Partition macros can reference tables invisible to a raw SQL parse, but any enrichment failure must retain the submitted schema rather than block creation. |
+| 2 | `script.has_unparseable_statement` is `False` | An opaque command plus a qualified `SELECT` exposes an incomplete table set, so derivation cannot be proven safe. |
+| 3 | `script.has_mutation()` is `False` | `statement.tables` reports query sources, not DML/DDL write targets; deriving from an `INSERT ... SELECT` source would ignore its target schema. |
+| 4 | `script.changes_default_schema()` is `False` | `USE other; SELECT * FROM t` and `SET search_path = other; ...` rebind runtime resolution, so any static answer is wrong. |
+| 5 | Empty-name parser artifacts are filtered, with at least one real table left | Table-valued functions such as `generate_series()` can produce a non-table artifact with an empty name. |
+| 6 | **Every real referenced table has an explicit schema** (zero unqualified refs) | The correctness-critical condition beyond the 2022 proposal. See below. |
+| 7 | No table catalog/schema identifier is quoted | Superset's `Table` value drops sqlglot's quote bit; on case-folding engines, quoted and unquoted identifiers with identical text can name different schemas. Conservatively retain the dropdown. |
+| 8 | Exactly one distinct **`(catalog, schema)` pair** — not one schema | `SELECT * FROM c1.s.t1 JOIN c2.s.t2` yields schema `{'s'}` (looks single!) but catalogs `{'c1','c2'}`. Schema alone is not a sufficient key. |
+| 9 | The script contains no `SHOW` metadata statement | A metadata command can expose a resolvable table target but has no meaningful virtual-dataset schema. |
+
+Otherwise: **keep today's dropdown value, unchanged.**
+
+### Why condition 6 is required (this is the important finding)
+
+The 2022 proposal was "if the query references exactly one explicit schema, use it." That
+rule is **unsafe** for mixed queries. Executed result:
+
+```
+SELECT * FROM public.t1 JOIN t2 ON t1.id = t2.id
+ → explicit={'public'} bare=['t2']
+```
+
+Exactly one explicit schema — so the naive rule stores `public`. But `t2` was resolving
+against `information_schema` (the dropdown) when the user ran it in SQL Lab, and would now
+resolve against `public`. **That silently changes what the query means** — and on
+Presto/Trino, what it actually executes. Requiring zero unqualified references makes the
+stored value provably inert: with nothing left to qualify, changing the default cannot
+change resolution. That also makes the fix order-independent w.r.t. the
+`raise_for_access` call below (`inferred`, follows from `Table.qualify`'s
+`self.schema or schema` semantics).
+
+### Verified parser behaviour matrix
+
+Executed against `superset/sql/parse.py` at the analyzed commit via an isolated harness
+outside the repository. `postgresql` dialect unless noted.
+
+| Query | Parsed | Rule fires? |
+|---|---|---|
+| `select * from public."Vehicle Sales"` *(the repro)* | `{Table("Vehicle Sales","public")}` | ✅ → `public` |
+| `SELECT * FROM my_table` | bare=`['my_table']` | ❌ zero → dropdown |
+| `SELECT * FROM a.t1 JOIN b.t2` | explicit=`{'a','b'}` | ❌ multi → dropdown |
+| `SELECT * FROM a.t1 JOIN a.t2` | explicit=`{'a'}` | ✅ → `a` |
+| `SELECT * FROM public.t1 JOIN t2` | explicit=`{'public'}`, bare=`['t2']` | ❌ **cond. 6** → dropdown |
+| `WITH orders AS (SELECT * FROM public.orders) SELECT * FROM orders` | `{Table("orders","public")}` — CTE correctly excluded | ✅ → `public` |
+| `SELECT * FROM public.t1 WHERE id IN (SELECT id FROM other.t2)` | explicit=`{'other','public'}` | ❌ multi → dropdown |
+| `SELECT * FROM (SELECT * FROM public.t1) sub` | explicit=`{'public'}` | ✅ → `public` |
+| `SELECT * FROM c1.s.t1 JOIN c2.s.t2` | schemas=`{'s'}`, catalogs=`{'c1','c2'}` | ❌ **cond. 8** → dropdown |
+| `SELECT * FROM public.t1 JOIN "public".t2` | quote bit is present only on the sqlglot AST | ❌ quoted-location guard → dropdown |
+| `USE other; SELECT * FROM t` | `changes_default_schema=True` | ❌ **cond. 4** → dropdown |
+| `SELECT * FROM {{ my_schema }}.t` | `SupersetParseError` | ❌ **cond. 1** → dropdown |
+| `SELECT 1` | no tables | ❌ zero → dropdown |
+| `SELECT * FROM \`proj.ds.tbl\`` (bigquery) | `Table("tbl","ds","proj")`, quoted location | ❌ quoted-location guard → dropdown |
+| `INSERT INTO secret.t SELECT * FROM public.s` | source=`public.s`, `has_mutation=True` | ❌ mutation guard → dropdown |
+| `EXPLAIN SELECT * FROM hidden.t; SELECT * FROM public.t1` | opaque first statement, enumerable=`public.t1` | ❌ unparseable guard → dropdown |
+| `SELECT * FROM public.t1 WHERE ds = '{{ presto.latest_partition("secret.audit") }}'` | Jinja-aware tables=`{public.t1, secret.audit}` | ❌ multi → dropdown |
+
+CTE handling is correct and needs no special-casing: `is_cte()`
+(`superset/sql/parse.py:2233-2250`) already excludes CTE names while still reporting a
+real table that shares a CTE's name.
+
+### Explicitly unchanged
+
+- **Zero-schema and multi-schema queries** keep today's dropdown behaviour. This is an
+ accepted limitation of the fix, not a defect to address here.
+- **Permissions / RLS enforcement** — untouched. `raise_for_access` keeps its current
+ arguments and semantics; permissions/RLS enforcement remains out of scope.
+- **The multi-schema dataset model** — deferred redesign, out of scope.
+- **Physical datasets** (`sql` empty) — untouched; `schema` remains required and authoritative.
+- **Frontend** — no change needed. The backend is the right and only place: it is the
+ convergence point for all three callers (SQL Lab modal, ResultSet panel, MCP tool).
+
+---
+
+## Latent Bugs Found
+
+- `superset/commands/dataset/update.py:206-217` — `PUT /api/v1/dataset/<id>` accepts a new
+ `schema` for a virtual dataset with the same no-reconciliation-against-`sql` gap; editing
+ a virtual dataset's SQL leaves a now-stale `schema`. Same defect class, separate entry point. (`verified`)
+- `superset/mcp_service/dataset/tool/create_virtual_dataset.py:130-131` — passes
+ `schema_name` through for virtual datasets. **Routes through `CreateDatasetCommand`, so
+ the fix above covers it automatically** — a convergence point, not a second fix site. (`verified`)
+- `superset/commands/dataset/duplicate.py:79` — `table.override(self._base_model)` copies
+ `schema` (in `export_fields`, `superset/connectors/sqla/models.py:1553`). Correct for a
+ duplicate, but propagates an already-wrong value. Not a defect in itself. (`verified`)
+- Dataset import (`superset/commands/dataset/importers/v1/utils.py:338,407`) trusts the
+ `schema` in the YAML for virtual datasets. Correct for round-tripping; perpetuates bad
+ values. Not a defect in itself. (`verified`)
+- **Identifier case folding is unmodelled anywhere in this path.** `SELECT * FROM
+ MySchema.tbl` stores `MySchema` verbatim, while Postgres folds the unquoted identifier to
+ `myschema` at execution. Pre-existing, affects the dropdown value equally, and broader
+ than this bug. (`verified` by execution)
+- `SELECT * FROM generate_series(1,10)` yields a table entry with an **empty-string name**.
+ The derivation logic filters this parser artifact before applying its safety conditions;
+ an empty-only reference set still falls back to the dropdown. (`verified`)
+- The quote guard cannot see quoting that appears only inside a partition-macro table
+ argument because macro extraction strips those quotes before returning `Table` values.
+ This is limited to partition-macro use on the Presto/Hive/Trino family and is deferred
+ to a separately scoped follow-up. (`verified by review`)
+- `process_jinja_sql()` can execute Jinja macros on the create path for users with
+ database access. The pre-existing `dataset_macro` missing-access-check issue is broader
+ than schema derivation and requires its own security ticket rather than a bundled fix.
+ (`verified by review`)
+- A schema-qualified table-valued function can be represented as an empty-name table and
+ is removed by the artifact filter. Distinguishing it safely from false table references
+ is deferred. (`verified by review`)
+
+---
+
+## Prevention
+
+1. **Backend regression guards are implemented** in
+ `tests/unit_tests/commands/dataset/test_create.py`, matching the file's existing
+ mock-based style (no live DB):
+ - `test_create_dataset_schema_derived_from_single_schema_query` — construct
+ `CreateDatasetCommand({"database": 1, "table_name": "d", "schema": "information_schema",
+ "sql": 'select * from public."Vehicle Sales"'})`, call `validate()`, assert
+ `command._properties["schema"] == "public"`. It failed on the pre-fix baseline
+ (`"information_schema"`) for exactly the intended reason.
+ - Companions pinning the fallbacks so the fix cannot over-reach:
+ `..._keeps_dropdown_schema_when_query_has_no_schema` (`SELECT * FROM t`),
+ `..._keeps_dropdown_schema_for_multi_schema_query` (`a.t1 JOIN b.t2`),
+ `..._keeps_dropdown_schema_for_unqualified_reference` (`public.t1 JOIN t2` — the
+ all-qualified guard), `..._keeps_dropdown_schema_when_sql_is_unparseable` (Jinja),
+ and `..._keeps_dropdown_schema_after_schema_change` (`USE`).
+ - Review hardening is pinned by cases for Jinja partition-macro tables, mutation and
+ opaque-statement guards, catalog differentiation, quoted schema identity, and
+ empty-name table-function artifacts. These cover the three additional safety
+ dimensions found in review: Jinja-aware table extraction, write-target blindness,
+ and case-folding ambiguity.
+ - Unit-level assertion on `_properties` rather than a persisted row keeps it a true
+ unit test; `validate()` already mutates `_properties["database"]`/`["catalog"]`, so
+ asserting on it follows the established contract.
+
+2. **Rule: a field that is *derived* from user-supplied SQL must never be accepted raw from
+ the client on a path that already parses that SQL.** `create.py` parsed the SQL for
+ security four lines below where it took `schema` on trust. A review checklist item —
+ "this endpoint parses `sql`; which other payload fields are answerable from the parse?" —
+ would have caught it in 2021.
+
+3. **Keep the frontend transport test unchanged.**
+ `SaveDatasetModal.test.tsx:249` asserts that the modal forwards the editor's schema in
+ the request payload. The backend remains responsible for reconciling that value with
+ the query, so this assertion remains factually correct and is not part of the fix.
+
+**Not preventable by monitoring** — the failure is silent and produces a working chart in
+the common (Postgres) case. Only an assertion at creation time catches it.
+
+---
+
+## Known Limitations of the Recommended Fix
+
+- **Case-differing schemas read as multi-schema.** `Public.t1 JOIN public.t2` →
+ `{'Public','public'}` → classified multi → falls back to the dropdown (`verified`).
+ Conservative and safe (never worse than today). Fixing it properly needs per-dialect
+ identifier folding (Postgres→lower, Snowflake/Oracle→upper, MySQL filesystem-dependent);
+ `sqlglot.optimizer.normalize_identifiers` could do it but needs dialect plumbing.
+ **Recommendation: leave as an explicit known limitation**, not worth the blast radius here.
+- **Case-preserved storage.** `MySchema.tbl` stores `MySchema` where Postgres would resolve
+ `myschema`. Pre-existing and equally true of the dropdown value today — not a regression.
+- **Quoted catalog/schema identifiers conservatively fall back.** This includes schema
+ names that require quoting because they contain dots or spaces. Superset's extracted
+ `Table` value does not retain the quote bit needed to compare their engine-specific
+ identity safely; retaining the dropdown is the safe status quo.
+- **Uniqueness-key shift.** Deriving `schema` before `Table(...)` at line 75 means the
+ uniqueness check keys on the derived value. That is intended and necessary — deriving it
+ *after* line 77 would let two datasets collide on `(table_name, public, catalog)`.
+ Reviewers should confirm this ordering is deliberate.
diff --git a/superset/commands/dataset/create.py b/superset/commands/dataset/create.py
index 310fa9b24d8..5336deb1f65 100644
--- a/superset/commands/dataset/create.py
+++ b/superset/commands/dataset/create.py
@@ -35,7 +35,8 @@ from superset.commands.utils import populate_subjects
from superset.daos.dataset import DatasetDAO
from superset.exceptions import SupersetParseError, SupersetSecurityException
from superset.extensions import security_manager
-from superset.sql.parse import Table
+from superset.models.helpers import json_to_dict
+from superset.sql.parse import process_jinja_sql, Table
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
@@ -72,6 +73,87 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
if not catalog:
catalog = self._properties["catalog"] = database.get_default_catalog()
+ if sql:
+ try:
+ template_params_raw: str | None = self._properties.get(
+ "template_params"
+ )
+ template_params = json_to_dict(template_params_raw or "")
+ parse_result = process_jinja_sql(
+ sql,
+ database,
+ template_params,
+ )
+ except Exception: # pylint: disable=broad-exception-caught
+ logger.debug(
+ "Keeping submitted dataset schema because optional SQL "
+ "template processing failed",
+ exc_info=True,
+ )
+ else:
+ script = parse_result.script
+ tables = {table for table in parse_result.tables if table.table}
+ if len(tables) != len(parse_result.tables):
+ logger.debug(
+ "Ignoring empty table-name parser artifacts while deriving "
+ "the dataset schema"
+ )
+
+ if script.has_unparseable_statement:
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL contains "
+ "an unparseable statement"
+ )
+ elif script.has_show_statement():
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL contains "
+ "a metadata statement"
+ )
+ elif script.has_mutation():
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL mutates "
+ "database state"
+ )
+ elif script.changes_default_schema():
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL changes "
+ "the default schema"
+ )
+ elif script.has_quoted_table_location():
+ logger.debug(
+ "Keeping submitted dataset schema because a quoted table "
+ "location may have case-sensitive identity"
+ )
+ elif not tables:
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL has no "
+ "table references"
+ )
+ elif any(table.schema is None for table in tables):
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL has an "
+ "unqualified table reference"
+ )
+ elif (
+ len(
+ qualified_table_locations := {
+ (table.catalog, table.schema) for table in tables
+ }
+ )
+ != 1
+ ):
+ logger.debug(
+ "Keeping submitted dataset schema because the SQL spans "
+ "multiple catalog/schema locations"
+ )
+ else:
+ derived_catalog, derived_schema = next(
+ iter(qualified_table_locations)
+ )
+ schema = self._properties["schema"] = derived_schema
+ if derived_catalog is not None:
+ catalog = self._properties["catalog"] = derived_catalog
+
table = Table(table_name, schema, catalog)
if not DatasetDAO.validate_uniqueness(database, table):
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index b1b93892d47..29ea7348ad2 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -630,6 +630,14 @@ class BaseSQLStatement(Generic[InternalRepresentation]):
"""
return False
+ def has_quoted_table_location(self) -> bool:
+ """Check whether a table has a quoted catalog or schema identifier."""
+ return False
+
+ def is_show_statement(self) -> bool:
+ """Check whether this is a SHOW metadata statement."""
+ return False
+
def get_disallowed_tables(
self,
tables: set[str],
@@ -1315,6 +1323,21 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
return True
return self.changes_search_path()
+ def has_quoted_table_location(self) -> bool:
+ """Return whether a table has a quoted catalog or schema identifier."""
+ for table in self._parsed.find_all(exp.Table):
+ for identifier_name in ("catalog", "db"):
+ identifier = table.args.get(identifier_name)
+ if isinstance(identifier, exp.Identifier) and identifier.args.get(
+ "quoted"
+ ):
+ return True
+ return False
+
+ def is_show_statement(self) -> bool:
+ """Return whether this is a SHOW metadata statement."""
+ return isinstance(self._parsed, exp.Show)
+
def get_disallowed_tables(
self,
tables: set[str],
@@ -2069,6 +2092,16 @@ class SQLScript:
"""
return any(statement.changes_default_schema() for statement in self.statements)
+ def has_quoted_table_location(self) -> bool:
+ """Check if any table has a quoted catalog or schema identifier."""
+ return any(
+ statement.has_quoted_table_location() for statement in self.statements
+ )
+
+ def has_show_statement(self) -> bool:
+ """Check if the script contains a SHOW metadata statement."""
+ return any(statement.is_show_statement() for statement in self.statements)
+
def optimize(self) -> SQLScript:
"""
Return optimized script.
diff --git a/tests/unit_tests/commands/dataset/test_create.py b/tests/unit_tests/commands/dataset/test_create.py
index 75174cf7505..64bcc7e51f4 100644
--- a/tests/unit_tests/commands/dataset/test_create.py
+++ b/tests/unit_tests/commands/dataset/test_create.py
@@ -26,10 +26,189 @@ from superset.exceptions import SupersetParseError
from superset.models.core import Database
+def _validate_virtual_dataset(
+ sql: str,
+ engine: str = "postgresql",
+ template_params: str | None = None,
+) -> CreateDatasetCommand:
+ """Validate a virtual dataset while isolating command dependencies."""
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.backend = engine
+ mock_database.db_engine_spec.engine = engine
+ mock_database.get_default_catalog.return_value = None
+
+ with (
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.get_database_by_id",
+ return_value=mock_database,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch("superset.commands.dataset.create.security_manager.raise_for_access"),
+ patch("superset.commands.dataset.create.populate_subjects"),
+ ):
+ properties = {
+ "database": 1,
+ "schema": "information_schema",
+ "table_name": "test_virtual_dataset",
+ "sql": sql,
+ }
+ if template_params is not None:
+ properties["template_params"] = template_params
+ command = CreateDatasetCommand(properties)
+ command.validate()
+
+ return command
+
+
+def test_create_dataset_schema_derived_from_single_schema_query() -> None:
+ """A single explicit query schema replaces the SQL Lab dropdown schema."""
+ command = _validate_virtual_dataset(
+ 'select * from public."Vehicle Sales"',
+ )
+
+ assert command._properties["schema"] == "public"
+
+
+def test_create_dataset_keeps_dropdown_schema_when_query_has_no_schema() -> None:
+ """An unqualified query keeps the SQL Lab dropdown schema."""
+ command = _validate_virtual_dataset("select * from t")
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_multi_schema_query() -> None:
+ """A query spanning multiple explicit schemas keeps the dropdown schema."""
+ command = _validate_virtual_dataset(
+ "select * from a.t1 union all select * from b.t2",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_unqualified_reference() -> None:
+ """A mixed qualified and bare query keeps the dropdown schema."""
+ command = _validate_virtual_dataset(
+ "select * from public.t1 join t2 on public.t1.id = t2.id",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_after_schema_change() -> None:
+ """A schema-changing statement prevents static schema derivation."""
+ command = _validate_virtual_dataset("use other; select * from public.t1")
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_when_sql_is_unparseable() -> None:
+ """A parse failure falls back to the SQL Lab dropdown schema."""
+ command = _validate_virtual_dataset("select * from {{ my_schema }}.t")
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_unparseable_statement() -> None:
+ """An opaque statement prevents derivation from an incomplete table set."""
+ command = _validate_virtual_dataset(
+ "explain select * from hidden.t; select * from public.t1",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_mutating_query() -> None:
+ """A mutating statement keeps the dropdown rather than its source schema."""
+ command = _validate_virtual_dataset(
+ "insert into secret.t select * from public.s",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_multi_catalog_query() -> None:
+ """The same schema in different catalogs is not a single location."""
+ command = _validate_virtual_dataset(
+ "select * from c1.s.t1 union all select * from c2.s.t2",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_quoted_schema() -> None:
+ """A quoted schema on a case-folding engine prevents unsafe derivation."""
+ command = _validate_virtual_dataset(
+ 'select * from "public".t1 union all select * from public.t2',
+ engine="snowflake",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_jinja_macro_table() -> None:
+ """Partition-macro tables participate in the single-location check."""
+ command = _validate_virtual_dataset(
+ "select * from public.t1 where ds = "
+ "'{{ presto.latest_partition(\"secret.audit\") }}'",
+ engine="presto",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_ignores_empty_table_function_reference() -> None:
+ """An empty parser artifact does not make a qualified query ambiguous."""
+ command = _validate_virtual_dataset(
+ "select * from public.t1 join generate_series(1, 10) on true",
+ )
+
+ assert command._properties["schema"] == "public"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_template_error() -> None:
+ """A Jinja failure in best-effort derivation keeps the dropdown schema."""
+ command = _validate_virtual_dataset("SELECT '{{' AS x FROM public.t")
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_schema_uses_template_params() -> None:
+ """Submitted template parameters determine the derived query schema."""
+ command = _validate_virtual_dataset(
+ "select * from {{ source | default('public.t') }}",
+ template_params='{"source": "secret.t"}',
+ )
+
+ assert command._properties["schema"] == "secret"
+
+
+def test_create_dataset_keeps_dropdown_schema_for_show_statement() -> None:
+ """A metadata statement has no meaningful virtual-dataset schema."""
+ command = _validate_virtual_dataset(
+ "show columns from foo from bar",
+ engine="mysql",
+ )
+
+ assert command._properties["schema"] == "information_schema"
+
+
+def test_create_dataset_derives_catalog_and_schema() -> None:
+ """A single three-part reference updates both catalog and schema."""
+ command = _validate_virtual_dataset("select * from prod.sales.orders")
+
+ assert command._properties["schema"] == "sales"
+ assert command._properties["catalog"] == "prod"
+
+
def test_create_dataset_invalid_sql_parse_error() -> None:
"""Test that invalid SQL returns a 4xx error when caught as SupersetParseError."""
mock_database = Mock(spec=Database)
mock_database.id = 1
+ mock_database.backend = "postgresql"
mock_database.db_engine_spec.engine = "postgresql"
mock_database.get_default_catalog.return_value = None
@@ -81,6 +260,7 @@ def test_create_dataset_valid_sql_with_access_error() -> None:
"""
mock_database = Mock(spec=Database)
mock_database.id = 1
+ mock_database.backend = "postgresql"
mock_database.db_engine_spec.engine = "postgresql"
mock_database.get_default_catalog.return_value = None