aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4380666460
##########
superset/models/helpers.py:
##########
@@ -4015,7 +4015,9 @@ def get_from_clause(
# Apply RLS filters to virtual dataset SQL to prevent RLS bypass
# For each table referenced in the virtual dataset, apply its RLS
filters
if parsed_script.statements:
- default_schema = self.database.get_default_schema(self.catalog)
+ default_schema = self.schema or self.database.get_default_schema(
Review Comment:
I fixed both cache-hit regressions in
a121abfb2b17aa4fd8aab1c0d56275252152229d.
Correction to my reply at
https://github.com/apache/superset/pull/45681#discussion_r4380159058 : “built
(not executed)” was incorrect. On no-join engines, building a series-limit
query can execute its prequery. Expression RLS keys now come from the
expressions themselves, without building the chart query. Both datasource cache
paths reuse the dataset SQL's resolved schema, and share memoized
default-catalog discovery. The separate ExtraCache build explicitly disables
prequery execution. Execution-time expression RLS remains in place.
Regression evidence (Docker, Python 3.11;
tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py):
- `test_cache_hit_schema_discovery_connection_budget`: 8 cases,
SqlaTable/Query × schema-less dynamic engine/inspector-needing static engine ×
ordinary expression/subquery. Real SQLite NullPool connection events: **3 → 1**
connections without a subquery, **4 → 1** with a subquery. The same 8 cases
pass against production source at 5c707e415e9db47ddaa89e40503fbe656c882feb with
the 1-connection budget.
- `test_cache_hit_series_limit_does_not_execute_prequery`: 3 cases using the
Druid no-join planner with SQLite transport, including ExtraCache. All fail at
57f71df01abfd0b8fba7445ad28f004e349b188e and pass at
a121abfb2b17aa4fd8aab1c0d56275252152229d; no prequery executes on the cache
hit. `test_series_limit_execution_still_runs_prequery` retains 2 passing
execution controls.
- `test_expression_cache_key_sibling_fields` (18),
`test_expression_cache_key_reference_precedence` (4),
`test_physical_expression_cache_key_schema_budget` (2),
`test_expression_cache_key_parse_failure_is_user_scoped` (2),
`test_expression_cache_key_ignores_unused_series_fields` (2), and
`test_expression_cache_key_reuses_catalog_discovery` (2) cover the
expression-only path, namespace/rendering edge cases, failure isolation and
discovery reuse.
Before: production source at 57f71df01abfd0b8fba7445ad28f004e349b188e with
the final tests, `pytest -q
tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py -k
'cache_hit_ or expression_cache_key or series_limit_execution_still' -m 'not
manual and not llm_compliance'`: **41 failed, 2 passed, 154 deselected** (exit
1).
After: `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'`: **237 passed** (exit 0), including all 194 existing
focused cases. The original 64-case
`test_virtual_dataset_avoids_schema_connections` matrix still asserts exactly
**0 connections on hits / 1 on misses**. Broader two-worker/loadfile Docker
run: **2344 passed, 3 skipped** (exit 0). Staged and all 16 branch-changed-file
pre-commit checks pass.
Siblings checked:
- superset/models/helpers.py:4032 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/models/helpers.py:4097 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/models/helpers.py:6325 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/connectors/sqla/models.py:2871 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/models/sql_lab.py:394 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/utils/rls.py:309 — fixed in
a121abfb2b17aa4fd8aab1c0d56275252152229d
- superset/common/query_context_processor.py:461 — not affected: synchronous
and async chart reads delegate to the tested datasource cache methods; no
separate SQL build for RLS keys.
- superset/models/helpers.py:2378 — not affected: explicit SQL generation
and actual chart execution keep prequery execution enabled; the 2 execution
controls pin that path.
- superset/utils/rls.py:251 — not affected: predicate lookup and dataset
exclusion policy are unchanged; only discovery is shared.
- superset/semantic_layers/models.py:885 — not affected: separate cache-key
implementation, no ExploreMixin query build.
- superset/db_engine_specs/couchbase.py — not affected: capability flags
reach the same guarded series-limit branch; no separate cache-key execution
implementation.
- superset/db_engine_specs/pinot.py — not affected: capability flags reach
the same guarded series-limit branch; no separate cache-key execution
implementation.
- superset/db_engine_specs/solr.py — not affected: capability flags reach
the same guarded series-limit branch; no separate cache-key execution
implementation.
- superset/db_engine_specs/elasticsearch.py — not affected: capability flags
reach the same guarded series-limit branch; no separate cache-key execution
implementation.
- superset/db_engine_specs/druid.py — not affected: capability flags reach
the same guarded series-limit branch; no separate cache-key execution
implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
