aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4392525969
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,156 @@ def get_rendered_sql(
def text(self, clause: str) -> TextClause:
return self.db_engine_spec.get_text_clause(clause)
+ def _get_rls_schema(self) -> str:
+ """Resolve RLS tables with the same schema policy as SQL
expressions."""
+ return get_rls_schema(
+ self.database,
+ self.catalog,
+ self.schema,
+ self.sql or "",
+ self.template_params_dict,
+ )
+
+ def _get_query_sql_expressions(
+ self, query_obj: QueryObjectDict, template_processor:
BaseTemplateProcessor
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ # Real column names take precedence over verbose names in filter
lookup.
+ columns = {col.verbose_name: col.expression for col in self.columns}
+ columns.update({col.column_name: col.expression for col in
self.columns})
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ expressions: list[str] = []
+
+ def add(value: Any, metric: bool = False) -> None:
+ """Resolve stored references in the namespace used by the query
sink."""
+ if isinstance(value, str):
+ references = metrics if metric else columns
+ expressions.append(references.get(value) or value)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ # Adhoc columns can name a saved calculated column after
+ # rendering. SQL metrics use their SQL literally instead.
+ expressions.append(
+ sql if "expressionType" in value else columns.get(sql)
or sql
Review Comment:
My earlier reply missed the GROUP BY raw-SQL path. The statement that
columns/GROUP BY use real names only was incorrect.
Fixed in aa1813a84f789496e71aa86ed6bc8e5643dc06fb. The collector
distinguishes raw GROUP BY SQL from saved bare names and quote-before-render
detail selections. It also follows ORDER BY-enabled grouping for series metrics
and the datasource-specific adhoc-column fallback. It does not render
already-rendered SQL again, and the earlier bare-name and removed_filters
regressions remain passing.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_raw_string_groupby_cache_keys_are_user_scoped`: 22 SqlaTable/Query
cases covering columns with a metric, direct legacy groupby, grouping enabled
by adhoc/saved ORDER BY metrics, JOIN/prequery series paths, and
literal-template output. Execution returns `[1]` and `[2]`; the incoming source
contributes `[[], []]` and equal full keys. The new collector contributes each
exact predicate, yields different keys, and renders each selection source once.
- `test_raw_string_detail_selection_matches_execution`: unknown detail
strings are quoted before one template pass, rather than treated like saved
bare names. Both datasource types assert exact render inputs and parsed SQL,
including a template whose rendered expression needs RLS.
- `test_orderby_grouping_collects_series_metric_rls`: active series metrics
contribute their predicates when ORDER BY enables grouping without selected
metrics.
- `test_quoted_adhoc_reference_fallback_cache_keys_are_user_scoped`: missing
`isColumnReference` metadata uses quoted original source, covering selected
columns, groupby, filters, ORDER BY labels and BASE_AXIS.
- `test_adhoc_sql_looking_saved_name_matches_datasource_execution`:
SqlaTable quotes a matching physical metadata name, while Query executes
literal adhoc SQL even on a metadata match.
Docker proof: with these final tests retained and production
helpers.py/models.py restored to bac4307d0cd0444a7f5883a80aedf49582984fbe
(SHA256 checked against `git show`), **34 failed, 347 passed**. At
aa1813a84f789496e71aa86ed6bc8e5643dc06fb, **381 passed** in the complete
module, retaining all 346 earlier cases. The one new before-pass is the Query
metadata-match compatibility control. Full `tests/unit_tests/connectors` and
`tests/unit_tests/models`, plus the affected schema-parameter tests, RLS
utilities and query-context processor tests: **1,966 passed**
(`manual`/`llm_compliance` deselected). Staged-file and final
branch-changed-file pre-commit pass; Docker changed-file Ruff passes. `docker
rm -fv pr45681-raw-tests` completed; `docker ps -a --filter name=pr45681` shows
only the header.
Siblings checked:
- superset/models/helpers.py:4066 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4135 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4118 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4184 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4024 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/connectors/sqla/models.py:2389 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:5474 — not affected: execution already renders
unknown GROUP BY strings as SQL; the collector is aligned with it, including
grouping activated by ORDER BY.
- superset/models/helpers.py:5534 — not affected: detail execution quotes
unknown strings before rendering; the new detail regression pins both literal
identifiers and SQL produced by that template pass.
- superset/models/helpers.py:5440 — not affected: unknown ORDER BY strings
are rejected rather than treated as SQL; adhoc SQL metrics and label-referenced
columns follow their separate tested namespaces.
- superset/models/helpers.py:5748 — not affected: unknown filter strings are
skipped, while saved columns, metrics and adhoc labels have explicit
resolution; existing filter/verbose-label/removed_filters tests pass.
- superset/connectors/sqla/models.py:2446 — not affected: execution already
renders an adhoc lookup, resolves saved metadata, then quotes the original
source for a missing isColumnReference fallback; the new collector override
mirrors those choices.
- superset/models/sql_lab.py:512 — not affected: Query executes adhoc SQL
literally even when its metadata name matches; the new default collector hook
preserves that contract and its Query control passes.
- superset/connectors/sqla/models.py:2562 — not affected: series metrics
remain SQL metrics or saved metric names, not column references; grouping
activation in the caller is the changed condition. Both JOIN and prequery
execution variants are tested.
- superset/models/helpers.py:4593 — not affected: the Query series-orderby
wrapper uses the same metric namespace; Query grouping and series tests cover
its caller.
- superset/connectors/sqla/models.py:2891 — not affected: the SqlaTable
cache wrapper delegates to the changed common collector and does not interpret
raw selection strings independently.
- superset/models/sql_lab.py:395 — not affected: the Query cache wrapper
delegates to the changed common collector; the raw-string two-user tests cover
both wrappers.
- superset/models/helpers.py:2146 — not affected: SQL metric execution
renders source once and disables rendering in the validation stage; the
existing single-render metric tests pass unchanged.
- superset/models/helpers.py:2180 — not affected: generic SQL execution
renders its own source, not collector output; no second pass was added to
expression predicate parsing.
- superset/models/helpers.py:3947 — not affected: datasource SQL is a
separate source collected independently from selected expressions; the new
probes deliberately use table-free datasource SQL.
- superset/models/helpers.py:5067 — not affected: expression-validation SQL
is execution/validation-only, not a chart cache consumer.
- superset/models/helpers.py:5160 — not affected: the legacy dict timestamp
helper is not used for TableColumn-based chart selections; the existing
temporal-column and removal regressions pass.
- superset/models/helpers.py:5182 — not affected: saved calculated-column
execution renders stored expressions, not their quoted names; existing
stored-expression and bare-name tests pass.
- superset/models/helpers.py:6253 — not affected: WHERE and HAVING enter the
collector as source SQL and remain single-rendered; the prior two-user controls
pass.
- superset/models/helpers.py:6296 — not affected: ORDER BY alias
literal_column wraps an already-collected expression label without rendering
new SQL.
- superset/connectors/sqla/models.py:1170 — not affected: dataset RLS clause
SQL is collected through the existing per-dataset RLS key, not through raw
selection fallback.
- superset/connectors/sqla/models.py:1182 — not affected: guest RLS clause
SQL remains on the existing guest/dataset RLS path.
- superset/connectors/sqla/models.py:1507 — not affected: saved column
source SQL is rendered separately; uncalculated saved names remain quoted,
never templated by the collector.
- superset/connectors/sqla/models.py:1598 — not affected: saved timestamp
expressions use their stored source; the existing effective-time and
removed-time tests pass.
- superset/connectors/sqla/models.py:1752 — not affected: saved metric
source SQL is rendered in the metric namespace, not the new adhoc-column hook.
- superset/connectors/sqla/models.py:1793 — not affected: normalized metric
SQL is derived from the stored metric source, not an unknown column-name
fallback.
- superset/connectors/sqla/models.py:2207 — not affected: fetch predicates
remain explicit SQL sources collected only when enabled; the prior
stored-expression control passes.
- superset/connectors/sqla/models.py:2352 — not affected: adhoc SQL metrics
stay literal; the column-specific hook is not called for them.
- superset/common/query_context_processor.py:461 — not affected: synchronous
chart cache consumers call the datasource wrapper with the same query dict;
there is no separate selection renderer.
- superset/common/query_context.py:157 — not affected: delegates cache-key
creation to QueryContextProcessor.
- superset/tasks/async_queries.py:372 — not affected: asynchronous keys use
the same QueryContext/datasource collector.
- superset/semantic_layers/models.py:886 — not affected: separate cache
implementation that does not inherit ExploreMixin.
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,201 @@ def get_rendered_sql(
def text(self, clause: str) -> TextClause:
return self.db_engine_spec.get_text_clause(clause)
+ def _get_rls_schema(self) -> str:
+ """Resolve RLS tables with the same schema policy as SQL
expressions."""
+ return get_rls_schema(
+ self.database,
+ self.catalog,
+ self.schema,
+ self.sql or "",
+ self.template_params_dict,
+ )
+
+ def _get_filter_column(
+ self,
+ name: str,
+ columns_by_name: dict[str, "TableColumn"],
+ ) -> Optional["TableColumn"]:
+ """Resolve real names before the first matching verbose filter
label."""
+ col = columns_by_name.get(name)
+ if col is not None:
+ return col
+ return next((col for col in self.columns if col.verbose_name == name),
None)
+
+ def _get_query_sql_expressions( # noqa: C901
+ self,
+ query_obj: QueryObjectDict,
+ template_processor: BaseTemplateProcessor,
+ granularity: str | None,
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ columns_by_name = {col.column_name: col for col in self.columns}
+ columns = {name: col.expression for name, col in
columns_by_name.items()}
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ selected_columns = query_obj.get("columns") or []
+ selected_metrics = query_obj.get("metrics") or []
+ adhoc_columns_by_label = {
+ col.get("label"): col
+ for col in selected_columns
+ if utils.is_adhoc_column(col)
+ }
+ metric_labels = {utils.get_metric_name(metric) for metric in
selected_metrics}
+ expressions: list[str] = []
+
+ def add(value: Any, metric: bool = False) -> None:
+ """Resolve stored references in the namespace used by the query
sink."""
+ if isinstance(value, str):
+ references = metrics if metric else columns
+ expressions.append(references.get(value) or value)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ # Adhoc columns can name a saved calculated column after
+ # rendering. SQL metrics use their SQL literally instead.
+ expressions.append(
+ sql if metric else columns.get(sql.strip()) or sql
+ )
+ add(value.get("column"))
+ add(value.get("column_name"))
+
+ def add_orderby(value: Any) -> None:
+ """ORDER BY prefers selected labels before stored metric/column
names."""
+ if isinstance(value, str):
+ if value in metric_labels:
+ # Selected metrics have already been collected.
+ return
+ if value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+ return
+ add(
+ value,
+ metric=utils.is_adhoc_metric(value)
+ or (isinstance(value, str) and value in metrics),
+ )
+
+ def add_filter(value: Any) -> None:
+ """Filters accept verbose column names before stored metric
names."""
+ if not isinstance(value, str):
+ add(value)
+ elif value == utils.DTTM_ALIAS and query_obj.get("is_timeseries",
True):
+ add(granularity)
+ elif (col := self._get_filter_column(value, columns_by_name)) is
not None:
+ expressions.append(col.expression or value)
+ elif value in metrics:
+ add(value, metric=True)
+ elif value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+
+ for value in query_obj.get("groupby") or selected_columns:
+ add(value)
+ for metric_value in selected_metrics:
+ add(metric_value, metric=True)
+ for orderby_value, _ascending in query_obj.get("orderby") or []:
+ add_orderby(orderby_value)
+ for flt in query_obj.get("filter") or []:
+ add_filter(flt.get("col"))
+ series_limit = query_obj.get("series_limit") or (
+ query_obj.get("timeseries_limit")
+ if query_obj.get("is_timeseries", True)
+ else 0
+ )
+ add(
+ (
+ query_obj.get("series_limit_metric")
+ or query_obj.get("timeseries_limit_metric")
+ )
+ if series_limit
+ else None,
+ metric=True,
+ )
+ add(granularity)
+ if (
+ granularity
+ and self.always_filter_main_dttm
+ and self.main_dttm_col in self.dttm_cols
+ and self.main_dttm_col != granularity
+ ):
+ add(self.main_dttm_col)
+ extras = query_obj.get("extras") or {}
+ expressions.extend(
+ extras[key] for key in ("where", "having") if extras.get(key)
+ )
+ if (
+ query_obj.get("apply_fetch_values_predicate")
+ and self.fetch_values_predicate
+ ):
+ expressions.append(self.fetch_values_predicate)
+ return expressions
+
+ def _get_query_rls_cache_keys(
+ self,
+ query_obj: QueryObjectDict,
+ default_schema: str | None = None,
+ resolve_catalog: Callable[[], str | None] | None = None,
+ ) -> list[str]:
+ """Collect expression-only RLS rules without building or executing SQL.
+
+ Only subquery expressions need table discovery. Reuse the
+ datasource SQL's resolved schema, or resolve it once for physical
datasets.
+ Do not exclude the dataset's own id: an independent expression
subquery is
+ not constrained by the outer query's RLS.
+ """
+ from superset.utils.rls import collect_rls_predicates_for_sql
+
+ if not query_obj or not is_feature_enabled("ALLOW_ADHOC_SUBQUERY"):
+ return []
+ resolve_catalog = resolve_catalog or
cache(self.database.get_default_catalog)
+ extras = query_obj.get("extras") or {}
+ granularity = query_obj.get("granularity")
+ if granularity is not None and granularity not in self.dttm_cols:
+ granularity = self.main_dttm_col
+ template_kwargs = {
+ key: query_obj.get(key)
+ for key in (
+ "columns",
+ "groupby",
+ "metrics",
+ "row_limit",
+ "row_offset",
+ "filter",
+ )
+ }
+ template_kwargs.update(
+ time_column=granularity,
+ time_grain=extras.get("time_grain_sqla"),
+ table_columns=[col.column_name for col in self.columns],
+ )
+ template_kwargs.update(self.template_params_dict)
+ template_processor = self.get_template_processor(**template_kwargs)
+ predicates: set[str] = set()
+ for expression in self._get_query_sql_expressions(
+ query_obj, template_processor, granularity
+ ):
+ expression = template_processor.process_template(expression)
Review Comment:
My earlier reply missed the GROUP BY raw-SQL path. The comment about quoting
bare column names incorrectly covered unknown GROUP BY strings; I replaced that
comment with the specific saved-name rule.
Fixed in aa1813a84f789496e71aa86ed6bc8e5643dc06fb. The collector
distinguishes raw GROUP BY SQL from saved bare names and quote-before-render
detail selections. It also follows ORDER BY-enabled grouping for series metrics
and the datasource-specific adhoc-column fallback. It does not render
already-rendered SQL again, and the earlier bare-name and removed_filters
regressions remain passing.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_raw_string_groupby_cache_keys_are_user_scoped`: 22 SqlaTable/Query
cases covering columns with a metric, direct legacy groupby, grouping enabled
by adhoc/saved ORDER BY metrics, JOIN/prequery series paths, and
literal-template output. Execution returns `[1]` and `[2]`; the incoming source
contributes `[[], []]` and equal full keys. The new collector contributes each
exact predicate, yields different keys, and renders each selection source once.
- `test_raw_string_detail_selection_matches_execution`: unknown detail
strings are quoted before one template pass, rather than treated like saved
bare names. Both datasource types assert exact render inputs and parsed SQL,
including a template whose rendered expression needs RLS.
- `test_orderby_grouping_collects_series_metric_rls`: active series metrics
contribute their predicates when ORDER BY enables grouping without selected
metrics.
- `test_quoted_adhoc_reference_fallback_cache_keys_are_user_scoped`: missing
`isColumnReference` metadata uses quoted original source, covering selected
columns, groupby, filters, ORDER BY labels and BASE_AXIS.
- `test_adhoc_sql_looking_saved_name_matches_datasource_execution`:
SqlaTable quotes a matching physical metadata name, while Query executes
literal adhoc SQL even on a metadata match.
Docker proof: with these final tests retained and production
helpers.py/models.py restored to bac4307d0cd0444a7f5883a80aedf49582984fbe
(SHA256 checked against `git show`), **34 failed, 347 passed**. At
aa1813a84f789496e71aa86ed6bc8e5643dc06fb, **381 passed** in the complete
module, retaining all 346 earlier cases. The one new before-pass is the Query
metadata-match compatibility control. Full `tests/unit_tests/connectors` and
`tests/unit_tests/models`, plus the affected schema-parameter tests, RLS
utilities and query-context processor tests: **1,966 passed**
(`manual`/`llm_compliance` deselected). Staged-file and final
branch-changed-file pre-commit pass; Docker changed-file Ruff passes. `docker
rm -fv pr45681-raw-tests` completed; `docker ps -a --filter name=pr45681` shows
only the header.
Siblings checked:
- superset/models/helpers.py:4066 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4135 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4118 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4184 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:4024 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/connectors/sqla/models.py:2389 — fixed in
aa1813a84f789496e71aa86ed6bc8e5643dc06fb
- superset/models/helpers.py:5474 — not affected: execution already renders
unknown GROUP BY strings as SQL; the collector is aligned with it, including
grouping activated by ORDER BY.
- superset/models/helpers.py:5534 — not affected: detail execution quotes
unknown strings before rendering; the new detail regression pins both literal
identifiers and SQL produced by that template pass.
- superset/models/helpers.py:5440 — not affected: unknown ORDER BY strings
are rejected rather than treated as SQL; adhoc SQL metrics and label-referenced
columns follow their separate tested namespaces.
- superset/models/helpers.py:5748 — not affected: unknown filter strings are
skipped, while saved columns, metrics and adhoc labels have explicit
resolution; existing filter/verbose-label/removed_filters tests pass.
- superset/connectors/sqla/models.py:2446 — not affected: execution already
renders an adhoc lookup, resolves saved metadata, then quotes the original
source for a missing isColumnReference fallback; the new collector override
mirrors those choices.
- superset/models/sql_lab.py:512 — not affected: Query executes adhoc SQL
literally even when its metadata name matches; the new default collector hook
preserves that contract and its Query control passes.
- superset/connectors/sqla/models.py:2562 — not affected: series metrics
remain SQL metrics or saved metric names, not column references; grouping
activation in the caller is the changed condition. Both JOIN and prequery
execution variants are tested.
- superset/models/helpers.py:4593 — not affected: the Query series-orderby
wrapper uses the same metric namespace; Query grouping and series tests cover
its caller.
- superset/connectors/sqla/models.py:2891 — not affected: the SqlaTable
cache wrapper delegates to the changed common collector and does not interpret
raw selection strings independently.
- superset/models/sql_lab.py:395 — not affected: the Query cache wrapper
delegates to the changed common collector; the raw-string two-user tests cover
both wrappers.
- superset/models/helpers.py:2146 — not affected: SQL metric execution
renders source once and disables rendering in the validation stage; the
existing single-render metric tests pass unchanged.
- superset/models/helpers.py:2180 — not affected: generic SQL execution
renders its own source, not collector output; no second pass was added to
expression predicate parsing.
- superset/models/helpers.py:3947 — not affected: datasource SQL is a
separate source collected independently from selected expressions; the new
probes deliberately use table-free datasource SQL.
- superset/models/helpers.py:5067 — not affected: expression-validation SQL
is execution/validation-only, not a chart cache consumer.
- superset/models/helpers.py:5160 — not affected: the legacy dict timestamp
helper is not used for TableColumn-based chart selections; the existing
temporal-column and removal regressions pass.
- superset/models/helpers.py:5182 — not affected: saved calculated-column
execution renders stored expressions, not their quoted names; existing
stored-expression and bare-name tests pass.
- superset/models/helpers.py:6253 — not affected: WHERE and HAVING enter the
collector as source SQL and remain single-rendered; the prior two-user controls
pass.
- superset/models/helpers.py:6296 — not affected: ORDER BY alias
literal_column wraps an already-collected expression label without rendering
new SQL.
- superset/connectors/sqla/models.py:1170 — not affected: dataset RLS clause
SQL is collected through the existing per-dataset RLS key, not through raw
selection fallback.
- superset/connectors/sqla/models.py:1182 — not affected: guest RLS clause
SQL remains on the existing guest/dataset RLS path.
- superset/connectors/sqla/models.py:1507 — not affected: saved column
source SQL is rendered separately; uncalculated saved names remain quoted,
never templated by the collector.
- superset/connectors/sqla/models.py:1598 — not affected: saved timestamp
expressions use their stored source; the existing effective-time and
removed-time tests pass.
- superset/connectors/sqla/models.py:1752 — not affected: saved metric
source SQL is rendered in the metric namespace, not the new adhoc-column hook.
- superset/connectors/sqla/models.py:1793 — not affected: normalized metric
SQL is derived from the stored metric source, not an unknown column-name
fallback.
- superset/connectors/sqla/models.py:2207 — not affected: fetch predicates
remain explicit SQL sources collected only when enabled; the prior
stored-expression control passes.
- superset/connectors/sqla/models.py:2352 — not affected: adhoc SQL metrics
stay literal; the column-specific hook is not called for them.
- superset/common/query_context_processor.py:461 — not affected: synchronous
chart cache consumers call the datasource wrapper with the same query dict;
there is no separate selection renderer.
- superset/common/query_context.py:157 — not affected: delegates cache-key
creation to QueryContextProcessor.
- superset/tasks/async_queries.py:372 — not affected: asynchronous keys use
the same QueryContext/datasource collector.
- superset/semantic_layers/models.py:886 — not affected: separate cache
implementation that does not inherit ExploreMixin.
--
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]
