[aminghadersohi]

SUMMARY

Review revision: 2723978ee47c6fe5d34c7db14b0b60997cf59f49.

Avoid redundant schema inspection for virtual datasets and unsaved SQL Lab Explore charts while retaining underlying-table and expression-subquery RLS cache contributions.

• Dynamic-schema engines use the configured schema; other engines prefer the discoverable execution schema, retaining the configured schema when discovery returns no value.
• Expression cache collection follows rendered/stripped saved-column lookup, separate column/metric namespaces, grouping and label precedence, and execution's effective time column.
• Dataset and expression collectors share schema/default-catalog discovery without building a chart query or executing series-limit prequeries. Actual chart execution retains its prequeries.
• Schema extractors tolerate missing/empty URI databases without suppressing discovery errors.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No UI change.

test_cache_hit_schema_discovery_connection_budget asserts exactly one discovery connection per cache hit. test_virtual_dataset_avoids_schema_connections retains zero connections for cached requests with dynamic configured schemas or statically known connection schemas. test_cache_hit_series_limit_does_not_execute_prequery asserts no series-limit prequeries on cache hits, including the ExtraCache path.

Druid planner tests use SQLite transport; no live external-engine latency claim is made.

TESTING INSTRUCTIONS

Docker, Python 3.11:

PYTHONPATH=.:superset-core/src python -m pytest -q \
  tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py \
  tests/unit_tests/db_engine_specs/test_schema_parameters.py \
  -m 'not manual and not llm_compliance'

• Focused modules: 270 passed.
• Broader SQL, engine, model, chart-cache and RLS suites: 8109 passed, 15 skipped, 3 xfailed; the final three added alias cases also pass in the focused run.
• Full pre-commit on all 47 branch-changed files, including mypy: exit 0, using repository-pinned Ruff 0.9.7.
• Fresh POT extraction: matches; this revision adds no translatable strings.
• Current regression selection against incoming production source dba4383d1c39acf2968a79c793a89b852978cc9d: 26 failed, 7 passed. The same selection passes after the change; it includes two real user objects with different mocked RLS rules and different executed results/cache keys.
• Current focused modules against PR merge-base production source 48254b641bc1861b191138d58ecf0fc2c9de6df5: 213 failed, 57 passed. Passing cases are compatibility/negative controls, not claimed as failure reproducers.

Real-LLM checks were not run.

ADDITIONAL INFORMATION

Expression entry points include selected/grouped columns, stored and SQL/SIMPLE metrics, filter columns, WHERE/HAVING, ordering, active series-limit metrics, temporal axes and fetch-value predicates. Parse-failure keys retain per-user isolation. No migrations, new feature flags, or public API/UI changes.

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

❌ Patch coverage is 50.75529% with 163 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.10%. Comparing base (a1ddcf9) to head (98d2d9f).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/models/helpers.py	45.81%	85 Missing and 25 partials ⚠️
superset/jinja_context.py	73.33%	13 Missing and 3 partials ⚠️
superset/sql/parse.py	6.66%	14 Missing ⚠️
superset/connectors/sqla/models.py	54.54%	5 Missing ⚠️
superset/db_engine_specs/couchbase.py	0.00%	2 Missing ⚠️
superset/db_engine_specs/drill.py	0.00%	2 Missing ⚠️
superset/db_engine_specs/hive.py	0.00%	2 Missing ⚠️
superset/db_engine_specs/mysql.py	0.00%	2 Missing ⚠️
superset/db_engine_specs/presto.py	0.00%	2 Missing ⚠️
superset/db_engine_specs/singlestore.py	0.00%	2 Missing ⚠️
... and 3 more

❗ There is a different number of reports uploaded between BASE (a1ddcf9) and HEAD (98d2d9f). Click for more details.

HEAD has 4 uploads less than BASE

Flag	BASE (a1ddcf9)	HEAD (98d2d9f)
python	7	3

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #45681       +/-   ##
===========================================
- Coverage   84.56%   65.10%   -19.47%     
===========================================
  Files        3068     3060        -8     
  Lines      204987   204420      -567     
  Branches    48171    47801      -370     
===========================================
- Hits       173340   133079    -40261     
- Misses      28346    68259    +39913     
+ Partials     3301     3082      -219     

Flag	Coverage Δ
hive	34.50% <10.57%> (-0.05%)	⬇️
mysql	?
postgres	?
presto	36.23% <13.59%> (-0.05%)	⬇️
python	52.38% <50.75%> (-35.26%)	⬇️
sqlite	52.27% <50.75%> (+0.03%)	⬆️
unit	?

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.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	33579b7
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ae182958ecc81a7fe101250
😎 Deploy Preview	https://deploy-preview-45681--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[aminghadersohi]

Acceptance run at e0dc5c9 against the merge-base 3c9f08d (base = the three changed files checked out from the merge-base, same stack).

Setup: docker-compose-light, Postgres 17 as the analytic DB (60k-row table), one dashboard per dataset with 10 charts and 3 cascading native filters. Connections counted twice and the counts agree exactly: a SQLAlchemy connect listener with call stacks, and Postgres log_connections (connection authorized ... database=<analytic db>). Users: admin, a Gamma user with RLS rules on the underlying physical tables, and an embedded guest token. Traffic: the chart-data requests a dashboard load sends, plus a Playwright run of the real UI (load, reload, cascade-select the three filters, open Explore).

Connections per dashboard load (13 chart-data requests), base → head

Dataset	cold	warm (all cache hits)	force refresh
virtual, schema set	33 → 10	13 → 0	39 → 13
virtual, schema set, Gamma + RLS	33 → 10	13 → 0	39 → 13
virtual, schema set, embedded guest	30 → 10	10 → 0	–
virtual, Jinja-templated SQL	46 → 10	26 → 0	52 → 13
virtual, impersonation on	33 → 10	13 → 0	39 → 13
physical	10 → 10	0 → 0	13 → 13
virtual, schema NULL	33 → 33	13 → 13	39 → 39

• Cache hits on a virtual dataset with a schema open no analytic connection at head; an uncached request opens one (the query). At base it was three per uncached request and one per cache hit (two with Jinja).
• Across the whole run Postgres logged 2480 logins at base and 1115 at head. Physical, schema-NULL and SQL Lab counts are unchanged.
• The browser run matches: reload 13 → 0, filter cascade 33 → 10, Explore 3 → 1.

Results unchanged: 1,434 base/head request pairs have the same status, cache key, SQL text, rows and is_cached. That covers admin, Gamma with RLS and guest, all variants above, SQL Lab sync, and query-backed charts (same Query ids on both builds, schema set and NULL). The RLS predicates appear in the same SQL at both. Schema NULL still falls back to the database default schema.

Default-schema lookup raising: at base every virtual-dataset chart fails with a 500. At head, schema-set virtual datasets (plain and Jinja) and schema-set query-backed charts return the same data. Schema-NULL ones still fail because they need the lookup.

Timing, with a simulated 0.7 s warehouse login per connection: warm load of the schema-set virtual dashboard 2.5–2.8 s → 0.3–0.6 s (median cache-hit request 0.88 s → 0.15 s), the same as the physical dashboard. Cold load 8.5 s → 3.2 s.

Pre-existing, outside this diff (not regressions, measured at head):

• find_datasets (superset/utils/rls.py:222) still calls get_default_schema once per cache hit when the virtual SQL reads a table whose physical dataset is registered with schema NULL. This PR halves it (26 → 13 per warm load) but does not remove it.
• get_default_catalog() (superset/utils/rls.py:317 and :90, superset/security/manager.py:5179) runs once or more per request. It opens no connection on Postgres or Snowflake, but per the engine specs it does on Databricks without connect_args.catalog, Doris without a catalog in the URI, and BigQuery/Datastore without a project in the URI. That part comes from reading the code; I did not measure it.

Not covered: a real Snowflake connection (counts are real Postgres logins, and the login cost is simulated), async SQL Lab (no Celery in the light stack), and CACHE_IMPERSONATION on.

screenshots: n/a (not a visual change)

[codeant-ai-for-open-source]

  CodeAnt PR Risk

🟢  Low Risk

• The PR appears safe to merge; no actionable issues were identified in the review.

Assessed commit: 98d2d9f7dc4b

[msyavuz]

Looks good to me but there are still unattended reviews from @sadpandajoe

[sadpandajoe]

This compares the raw groupby strings to series_columns, but execution matches on the rendered label (outer.name in series_column_labels). With groupby=["{{ dim }}"], dim="col1" and series_columns=["col1"], execution runs the series-limit metric while this check sees no overlap, so has_series_groupby is false and a restricted-table sub-query used only in series_limit_metric contributes no predicates to the cache key. Two viewers with different RLS on that table could then share a cached result. Should the labels be compared after the same template rendering execution applies?

[aminghadersohi]

Good catch, confirmed. Grouped execution labels a non-saved string selection with its processed SQL (_process_select_expression: render, then sanitize_clause, then _unwrap_select_clause), and matches that outer.name against series_column_labels. The collector compared the raw string.

Fixed in 98d2d9f: has_series_groupby derives each unknown string's label from the same single render_for_discovery pass used to collect it, then applies sanitize_clause / _unwrap_select_clause as execution does. Fail-closed cases count as overlapping: an opaque render, a label with a subquery (RLS rewrites it per viewer), and engines without SELECT aliases (execution names the column by its SQL there).

Tests (fail before, pass after; SqlaTable and Query):

• test_cache_key_collects_series_metric_for_rendered_groupby_labels ({{ dim }} with dim=col1, col1;, {{ 'col1' }} ;)
• test_cache_key_collects_series_metric_without_select_aliases
• test_cache_key_collects_series_metric_for_opaque_groupby_label

Siblings checked:

• superset/models/helpers.py:4269 (GROUP BY saved columns, granularity, adhoc columns) — not affected: execution labels them by raw name / get_column_name, unrendered.
• superset/models/helpers.py:4216 (add_orderby: metric_labels, adhoc_columns_by_label, saved names) — not affected: execution keys on get_metric_name / adhoc label / names, all unrendered.
• superset/models/helpers.py:4252 (need_groupby ORDER BY check) — not affected: same unrendered metric keys as execution.
• superset/models/helpers.py:4231 (add_filter: _get_filter_column, metric names, adhoc labels, removed_filters) — not affected: execution resolves the same raw strings.
• superset/models/helpers.py:4406 (series prequery columns) — not affected: built from the same raw selections and re-collected by the same code.

test_raw_string_groupby_cache_keys_are_user_scoped still pins a single render of the source.