aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4399364963
##########
superset/models/helpers.py:
##########
@@ -4104,6 +4110,365 @@ 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,
+ render: Callable[[str], str | None],
+ ) -> 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],
+ opaque_templates: list[str],
+ prequery_objs: list[QueryObjectDict],
+ ) -> list[str]:
+ """Render SQL-bearing chart fields without building a chart or its
prequeries.
+
+ Render each source expression once, matching execution, but through
+ ``render_for_discovery`` so no macro runs before the cache lookup. An
+ expression that needs a macro is recorded in ``opaque_templates``. An
+ adhoc column that resolves to saved metadata also renders that column's
+ expression. A series-limit prequery is appended to ``prequery_objs``.
+ """
+ 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 render(expression: str) -> str | None:
+ """Render like execution without running macros; None when
opaque."""
+ # Only FROM's removals may skip filter columns: execution renders
+ # FROM first, so they are a subset of its own and collection can
+ # only over-include. Removals made by a chart expression are
+ # discarded for the same reason.
+ removed = list(removed_filters)
+ try:
+ return template_processor.render_for_discovery(expression)
+ except Exception: # pylint: disable=broad-except # noqa: BLE001
+ # Opaque (a stubbed macro was used) or invalid: either way the
+ # SQL execution produces is unknown here.
+ opaque_templates.append(expression)
+ return None
+ finally:
+ removed_filters[:] = removed
+
+ def add_expression(expression: str) -> None:
+ """Render source SQL, preserving literal Jinja in its output."""
+ if (rendered := render(expression)) is not None:
+ expressions.append(rendered)
+
+ 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")) and (
+ sql := render(sql)
+ ) is not None:
+ if not metric:
+ sql = self._get_adhoc_column_rls_expression(
+ cast(AdhocColumn, value), sql, render
+ )
+ 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. Discovery runs the request-only filter readers for real, so
+ # their removals reach this collector. A macro aborts the render early,
+ # recording fewer removals, which only over-includes expressions.
+ if self.sql:
+ try:
+
template_processor.render_for_discovery(self.sql.strip("\t\r\n; "))
+ except Exception: # pylint: disable=broad-except # noqa: BLE001,
S110
+ pass
+ collect_time_column = query_obj.get("is_timeseries", True) or (
+ granularity not in removed_filters
+ and utils.DTTM_ALIAS not in removed_filters
+ )
+ if collect_time_column:
+ 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
+ and self.main_dttm_col not in removed_filters
+ ):
+ add(self.main_dttm_col)
+ for flt in query_obj.get("filter") or []:
+ if all(flt.get(key) for key in ("col", "op")):
+ add_filter(flt["col"])
+ extras = query_obj.get("extras") or {}
+ for key in ("where", "having"):
+ if extras.get(key):
+ add_expression(extras[key])
+ if (
+ query_obj.get("apply_fetch_values_predicate")
+ and self.fetch_values_predicate
+ ):
+ add_expression(self.fetch_values_predicate)
+ series_limit = query_obj.get("series_limit") or (
+ query_obj.get("timeseries_limit")
+ if query_obj.get("is_timeseries", True)
+ else 0
+ )
+ series_columns = {
+ self.db_engine_spec.make_label_compatible(name)
+ for name in utils.get_column_names(query_obj.get("series_columns"))
+ }
+ has_series_groupby = bool(
+ need_groupby
+ and effective_columns
+ and (
+ (query_obj.get("is_timeseries", True) and not series_columns)
+ or series_columns.intersection(
Review Comment:
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 98d2d9f7dc4b3d2455226fd0646302351ed156b5: `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.
--
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]
