SEPURI-SAI-KRISHNA commented on code in PR #45431:
URL: https://github.com/apache/superset/pull/45431#discussion_r4361490204
##########
superset/connectors/sqla/models.py:
##########
@@ -2593,26 +2593,86 @@ def has_extra_cache_key_calls(self, query_obj:
QueryObjectDict) -> bool: # noqa
templatable_statements.append(extras["where"])
if "having" in extras:
templatable_statements.append(extras["having"])
- if columns := query_obj.get("columns"):
- calculated_columns: dict[str, Any] = {
- c.column_name: c.expression for c in self.columns if
c.expression
- }
- for column_ in columns:
- if utils.is_adhoc_column(column_):
- templatable_statements.append(column_["sqlExpression"])
- elif isinstance(column_, str) and column_ in
calculated_columns:
- templatable_statements.append(calculated_columns[column_])
- if metrics := query_obj.get("metrics"):
- metrics_by_name: dict[str, Any] = {
- m.metric_name: m.expression for m in self.metrics
- }
- for metric in metrics:
- if utils.is_adhoc_metric(metric) and (
- sql := metric.get("sqlExpression")
- ):
- templatable_statements.append(sql)
- elif isinstance(metric, str) and metric in metrics_by_name:
- templatable_statements.append(metrics_by_name[metric])
+ calculated_columns: dict[str, Any] = {
+ c.column_name: c.expression for c in self.columns if c.expression
+ }
+ calculated_columns_by_verbose_name: dict[str, Any] = {
+ c.verbose_name: c.expression
+ for c in self.columns
+ if c.expression and c.verbose_name
+ }
+ metrics_by_name: dict[str, Any] = {
+ m.metric_name: m.expression for m in self.metrics
+ }
+
+ def column_expression(column_: Any) -> Any:
+ """The templatable expression behind an adhoc or calculated
column."""
+ if utils.is_adhoc_column(column_):
+ return column_["sqlExpression"]
Review Comment:
Confirmed and taken. The same calculated column, reached two ways:
```
columns: ["username"] gate=True
keys=['alice']
columns: [{"label": "username", "sqlExpression": "username"}] gate=False
keys=[]
```
Both render `SELECT ('alice') AS username`, so the adhoc form put every user
on one cache entry.
Used `or ""` ahead of the `strip()`, since `is_adhoc_column` only checks
that the key is present. Kept the raw expression as the lookup default:
dropping it breaks `test_extra_cache_keys_in_adhoc_metrics_and_columns`, the
case where the adhoc expression carries the macro itself.
Checking the rest of the same class turned up one more: `groupby` is
rendered through this same lookup and was not inspected either, so a calculated
column grouped on leaked the same way. Gated with `columns`. `series_columns`
does not need it, since naming a calculated column there does not put its
expression in the SQL.
On the drift question, agreed, and I would rather not attach it to this PR.
Happy to look at it separately.
##########
superset/connectors/sqla/models.py:
##########
@@ -2593,26 +2593,86 @@ def has_extra_cache_key_calls(self, query_obj:
QueryObjectDict) -> bool: # noqa
templatable_statements.append(extras["where"])
if "having" in extras:
templatable_statements.append(extras["having"])
- if columns := query_obj.get("columns"):
- calculated_columns: dict[str, Any] = {
- c.column_name: c.expression for c in self.columns if
c.expression
- }
- for column_ in columns:
- if utils.is_adhoc_column(column_):
- templatable_statements.append(column_["sqlExpression"])
- elif isinstance(column_, str) and column_ in
calculated_columns:
- templatable_statements.append(calculated_columns[column_])
- if metrics := query_obj.get("metrics"):
- metrics_by_name: dict[str, Any] = {
- m.metric_name: m.expression for m in self.metrics
- }
- for metric in metrics:
- if utils.is_adhoc_metric(metric) and (
- sql := metric.get("sqlExpression")
- ):
- templatable_statements.append(sql)
- elif isinstance(metric, str) and metric in metrics_by_name:
- templatable_statements.append(metrics_by_name[metric])
+ calculated_columns: dict[str, Any] = {
+ c.column_name: c.expression for c in self.columns if c.expression
+ }
+ calculated_columns_by_verbose_name: dict[str, Any] = {
+ c.verbose_name: c.expression
+ for c in self.columns
+ if c.expression and c.verbose_name
+ }
+ metrics_by_name: dict[str, Any] = {
+ m.metric_name: m.expression for m in self.metrics
+ }
+
+ def column_expression(column_: Any) -> Any:
+ """The templatable expression behind an adhoc or calculated
column."""
+ if utils.is_adhoc_column(column_):
+ return column_["sqlExpression"]
+ if isinstance(column_, str):
+ return calculated_columns.get(column_)
+ return None
+
+ def metric_expression(metric: Any) -> Any:
+ """The templatable expression behind an adhoc or saved metric."""
+ if utils.is_adhoc_metric(metric):
+ if sql_expression := metric.get("sqlExpression"):
+ return sql_expression
+ # A SIMPLE adhoc metric has no expression of its own, but
+ # ``adhoc_metric_to_sqla`` wraps its aggregate around the
+ # column's rendered expression, so a macro in a calculated
+ # column reaches the SQL through it.
+ metric_column = metric.get("column") or {}
+ column_name = metric_column.get("column_name")
+ return calculated_columns.get(column_name) if column_name else
None
+ if isinstance(metric, str):
+ return metrics_by_name.get(metric)
+ return None
+
+ def filter_expression(flt: Any) -> Any:
+ """The templatable expression a filter's ``col`` resolves to."""
+ col = flt.get("col")
+ if expression := column_expression(col):
+ return expression
+ if isinstance(col, str):
+ # A filter column that is not a column name is resolved against
+ # the columns' verbose names (right-click "Drill to detail by"
+ # passes those) and then against the saved metrics, for a
+ # filter that becomes a HAVING clause.
+ return calculated_columns_by_verbose_name.get(
+ col
+ ) or metrics_by_name.get(col)
+ return None
+
+ for column_ in query_obj.get("columns") or []:
+ if expression := column_expression(column_):
+ templatable_statements.append(expression)
+ for metric in query_obj.get("metrics") or []:
+ if expression := metric_expression(metric):
+ templatable_statements.append(expression)
+ # ``get_sqla_query`` renders these through the same template processor
as
+ # the columns and metrics above, so a macro in them must trigger the
+ # extraction too, otherwise two users whose rendered SQL differs share
a
+ # cache key:
+ # - an adhoc metric in ``orderby``, or an ``orderby`` entry naming a
+ # saved metric or calculated column not selected by the query
+ # - ``series_limit_metric`` (and its legacy
``timeseries_limit_metric``
+ # alias), via ``_get_series_orderby``
+ # - a filter whose ``col`` resolves to a calculated column or a saved
+ # metric
+ for order_by in query_obj.get("orderby") or []:
+ entry = order_by[0] if isinstance(order_by, (list, tuple)) else
order_by
+ if expression := metric_expression(entry) or
column_expression(entry):
+ templatable_statements.append(expression)
+ for limit_metric in (
+ query_obj.get("series_limit_metric"),
+ query_obj.get("timeseries_limit_metric"),
+ ):
+ if expression := metric_expression(limit_metric):
+ templatable_statements.append(expression)
+ for flt in query_obj.get("filter") or []:
+ if isinstance(flt, dict) and (expression :=
filter_expression(flt)):
+ templatable_statements.append(expression)
Review Comment:
Confirmed, with one more hop in it.
Your version catches a `granularity` that names the calculated column. It
misses a `granularity` naming a column that is not temporal: `get_sqla_query`
swaps in `main_dttm_col` (`helpers.py:4837`), so the expression that gets
rendered belongs to a column the query never named. With `main_dttm_col="mine"`
carrying `DATE('{{ current_username() }}')`, both forms gave `keys=[]` while
the SQL went `DATE('alice')` against `DATE('bob')`.
So the block follows that fallback:
```python
if granularity := query_obj.get("granularity"):
if granularity not in self.dttm_cols:
granularity = self.main_dttm_col
if granularity and (expression := calculated_columns.get(granularity)):
templatable_statements.append(expression)
```
--
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]
