aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4396198533
##########
superset/models/helpers.py:
##########
@@ -4104,6 +4110,286 @@ 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_adhoc_column_rls_expression(
+ self,
+ col: AdhocColumn,
+ rendered_sql: str,
+ template_processor: BaseTemplateProcessor,
+ ) -> str | None:
+ """Query-backed adhoc columns execute their rendered SQL literally."""
+ return rendered_sql
+
+ def _get_query_sql_expressions( # noqa: C901
+ self,
+ query_obj: QueryObjectDict,
+ template_processor: BaseTemplateProcessor,
+ granularity: str | None,
+ removed_filters: list[str],
+ ) -> list[str]:
+ """Render SQL-bearing chart fields without building a chart or its
prequeries.
+
+ Render each source expression once, matching execution. An adhoc column
+ that resolves to saved metadata also renders that column's expression.
+ """
+ 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_expression(expression: str) -> None:
+ """Render source SQL, preserving literal Jinja in its output."""
+ expressions.append(template_processor.process_template(expression))
+
+ 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
+ # Saved columns without expressions are quoted identifiers.
+ # Unknown selection strings have a separate SQL fallback below.
+ if expression := references.get(value):
+ add_expression(expression)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ if not metric:
+ sql = self._get_adhoc_column_rls_expression(
+ cast(AdhocColumn, value), sql, template_processor
+ )
+ if sql:
+ expressions.append(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."""
+ # Execution renders adhoc/metric SQL before checking removal, but
+ # delays saved-column SQL until after that check.
+ if not isinstance(value, str):
+ add(value)
+ elif value == utils.DTTM_ALIAS and query_obj.get("is_timeseries",
True):
+ if value not in removed_filters and granularity not in
removed_filters:
+ add(granularity)
+ elif (col := self._get_filter_column(value, columns_by_name)) is
not None:
+ if value not in removed_filters and col.expression:
+ add_expression(col.expression)
+ elif value in metrics:
+ add(value, metric=True)
+ elif value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+
+ effective_columns = query_obj.get("groupby") or [
+ col for col in selected_columns if col != utils.DTTM_ALIAS
+ ]
+ # ORDER BY metrics can enable grouping even without selected metrics.
+ need_groupby = bool(
+ query_obj.get("metrics") is not None
+ or query_obj.get("groupby")
+ or any(
+ utils.is_adhoc_metric(cast(Metric, value))
+ or (
+ isinstance(value, str)
+ and (
+ value in metric_labels
+ or (value in metrics and value not in
adhoc_columns_by_label)
+ )
+ )
+ for value, _ascending in query_obj.get("orderby") or []
+ )
+ )
+ for value in effective_columns:
+ if isinstance(value, str) and value not in columns:
+ # GROUP BY executes unknown strings as SQL. Detail selections
+ # quote them before rendering, unlike saved bare column names.
+ add_expression(
+ value if need_groupby else
self.database.quote_identifier(value)
+ )
+ else:
+ 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)
+ # FROM templates remove filters before execution renders time/filter
columns.
+ # Render with the same processor so its removal state reaches this
collector.
+ if self.sql:
+ self.get_rendered_sql(template_processor)
Review Comment:
Correction to my previous reply at
https://github.com/apache/superset/pull/45681#discussion_r4396010177 : two
paths remained. Calculated filter/time expressions without subqueries still
caused FROM rendering, and expression templates were rendered before checking
for subqueries. The SqlaTable saved-column resolution I labeled “not affected”
was affected too.
Fixed in 1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9. The collector inspects
SQL and literal template inputs without executing them, and renders only for
discovered table-reading subqueries. Quoted `SELECT` text and tableless
subqueries do not require FROM removal state. Opaque or ambiguous outputs use
the existing per-user/guest fallback key without running their macros,
preserving cache isolation. Execution-time rendering and the existing
ExtraCache gate are unchanged.
`test_cache_key_skips_partition_templates_without_subqueries` covers all
requested paths: calculated-column filters, non-timeseries calculated time
columns, extras.where/having, calculated columns, saved metrics, adhoc SQL,
fetch_values_predicate and adhoc saved-column resolution. Its 80 cases cover
SqlaTable and Query, successful/failing partition discovery, plus time aliases,
primary time filters, ordering, grouping, series metrics, quoted SELECT
literals and tableless subqueries. It checks zero partition calls and includes
a live-macro control.
Additional tests:
`test_opaque_expression_cache_key_is_viewer_scoped_without_rendering` checks
distinct viewer keys and executed RLS results;
`test_cache_key_inspection_does_not_execute_jinja_filters` and
`test_unused_template_parameter_subquery_does_not_trigger_macro` guard the
inspection itself. `test_nested_template_parameter_subquery_cache_keys` retains
exact RLS keys for literal/parameter-generated SQL.
With both production helpers restored to
90d1e8a992f2dc98d2652cf9ad53b4d757fbe5f3, the changed-test selection gives **94
failed, 6 passed**. The six passing cases are compatibility controls, not
failure reproducers. After restoring this implementation, all **100** selected
cases pass; the full touched file has **506 passed**. Requested suites plus
schema and RLS helper tests: **5,786 passed**; SQL coverage gate: **3,126
passed, 100.00%**. Existing swapped-RLS and removed-filter/series-limit
regressions pass. Branch-scoped pre-commit and mypy pass.
Siblings checked:
- superset/models/helpers.py:4144 — fixed in
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9 (side-effect-free template-input and
SQL inspection)
- superset/models/helpers.py:4231 — fixed in
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9 (columns, filters, temporal axes,
metrics, ordering, series limits, WHERE/HAVING and fetch predicates)
- superset/connectors/sqla/models.py:2572 — fixed in
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9 (resolved saved-column expressions are
checked too)
- superset/connectors/sqla/models.py:3196 — fixed in
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9 (SqlaTable uses the shared collector)
- superset/models/sql_lab.py:395 — fixed in
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9 (Query uses the shared collector; it
does not execute saved-column metadata)
- superset/connectors/sqla/models.py:3034 — not affected: the existing
ExtraCache-gated SQL build is unchanged.
- superset/models/helpers.py:4598 — not affected: execution-time FROM
rendering remains necessary and unchanged.
- superset/utils/rls.py:38 — not affected: the existing identity helper
distinguishes users and guest RLS scopes; the new opaque-template fallback
reuses it.
- superset/utils/rls.py:309 — not affected: underlying-table collection
parses SQL rather than executing expression templates.
- superset/sql/parse.py:3883 — not affected: schema-validation rendering
already neutralizes partition macros.
--
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]
