gabotorresruiz commented on code in PR #45431:
URL: https://github.com/apache/superset/pull/45431#discussion_r4318531360
##########
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:
Same class, also not a blocker: `granularity` is in `QueryObject.to_dict()`,
and `get_sqla_query` passes the resolved temporal column through
`get_timestamp_expression`, which renders its expression
(`models.py:1354-1357`). So a calculated temporal column carrying a macro
reaches the SQL and the time filter without contributing anything here. I
measured it on your branch: SQL differs per principal, keys identical.
Three lines if you want it in the same PR, and the file's suite stayed green
for me with it:
```suggestion
for flt in query_obj.get("filter") or []:
if isinstance(flt, dict) and (expression :=
filter_expression(flt)):
templatable_statements.append(expression)
# ``get_timestamp_expression`` renders the temporal column's
expression.
if granularity := query_obj.get("granularity"):
if expression := calculated_columns.get(granularity):
templatable_statements.append(expression)
```
##########
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:
Not a blocker and this predates the PR, but `column_expression` is now the
shared resolver for `filter` too, so it is worth one more hop here.
`adhoc_column_to_sqla` renders the expression and looks it up against the
dataset's columns first (`models.py:2197-2206`), so an adhoc column whose
`sqlExpression` is just the name of a calculated column renders *that column's*
expression rather than this string. `normalizeTimeColumn` builds exactly that
shape for an `x_axis` column reference (`normalizeTimeColumn.ts:64-71`). I
checked it on your branch: `"username"` as a plain string is detected,
`{"label": "username", "sqlExpression": "username"}` is not, and the rendered
SQL differs per user either way. One extra lookup covers both `columns` and the
`filter` path you just added:
```suggestion
def column_expression(column_: Any) -> Any:
"""The templatable expression behind an adhoc or calculated
column."""
if utils.is_adhoc_column(column_):
expression = column_["sqlExpression"]
# ``adhoc_column_to_sqla`` resolves the expression against
the
# dataset's columns first, so an adhoc column that only
names a
# calculated column renders that column's expression instead.
return calculated_columns.get(expression.strip(), expression)
```
Separate and much larger question, definitely not for this PR: this gate has
drifted from the renderer four times now (#30549, #30715, #30735, this one). Is
there a cheap way to make the next drift loud instead of silent, something like
having `get_sqla_query` record the expressions it handed to the template
processor and a test asserting `has_extra_cache_key_calls` inspects the same
set? Happy to dig into that with you if it sounds worth trying.
##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -1073,6 +1104,85 @@ def test_extra_cache_keys_in_adhoc_metrics_and_columns(
assert extra_cache_keys == expected_cache_keys
[email protected]("app_context")
[email protected](
+ "query_obj_fragment,has_extra_cache_keys",
+ [
+ ({"orderby": [[ADHOC_JINJA_METRIC, False]]}, True),
+ ({"orderby": [[ADHOC_PLAIN_METRIC, False]]}, False),
+ ({"orderby": [[SIMPLE_JINJA_METRIC, False]]}, True),
+ ({"orderby": [[SIMPLE_PLAIN_METRIC, False]]}, False),
+ ({"orderby": [["variable_profit", False]]}, True),
+ ({"orderby": [["plain_total", False]]}, False),
+ ({"series_limit_metric": ADHOC_JINJA_METRIC}, True),
+ ({"series_limit_metric": ADHOC_PLAIN_METRIC}, False),
+ ({"series_limit_metric": SIMPLE_JINJA_METRIC}, True),
+ ({"timeseries_limit_metric": ADHOC_JINJA_METRIC}, True),
+ ({"metrics": [SIMPLE_JINJA_METRIC]}, True),
+ ({"metrics": [SIMPLE_PLAIN_METRIC]}, False),
+ ({"filter": [{"col": ADHOC_JINJA_COLUMN, "op": "==", "val": 1}]},
True),
+ ({"filter": [{"col": ADHOC_PLAIN_COLUMN, "op": "==", "val": 1}]},
False),
+ ({"filter": [{"col": "username", "op": "==", "val": 1}]}, True),
+ ({"filter": [{"col": "Who am I", "op": "==", "val": 1}]}, True),
+ ({"filter": [{"col": "variable_profit", "op": "==", "val": 1}]}, True),
+ ({"filter": [{"col": "plain_total", "op": "==", "val": 1}]}, False),
+ ({"filter": [{"col": "user", "op": "==", "val": 1}]}, False),
+ ],
+)
+def test_extra_cache_keys_in_orderby_series_limit_and_filter(
+ query_obj_fragment: dict[str, Any],
+ has_extra_cache_keys: bool,
+):
+ """
+ `get_sqla_query` renders `orderby` metrics, `series_limit_metric` and
filter
+ columns through the template processor, so a macro in any of them has to
+ trigger the extra cache key extraction. Without that, two users whose
+ rendered SQL differs share one cache entry.
+
+ Each field is resolved the way `get_sqla_query` resolves it: an adhoc
+ expression, a SIMPLE adhoc metric's column, or a name looked up against the
+ dataset's metrics and calculated columns. A filter column also resolves
+ against the columns' verbose names and the saved metrics.
+ """
+ table = SqlaTable(
+ table_name="test_has_no_extra_cache_keys_table",
+ sql="SELECT 'abc' as user",
+ database=get_example_database(),
+ columns=[
+ TableColumn(column_name="user", type="VARCHAR(255)"),
+ TableColumn(
+ column_name="username",
+ type="VARCHAR(255)",
+ verbose_name="Who am I",
+ expression="{{ current_username() }}",
+ ),
+ ],
+ metrics=[
+ SqlMetric(
+ metric_name="variable_profit",
+ expression="SUM(price) * {{ url_param('multiplier') }}",
+ ),
+ SqlMetric(metric_name="plain_total", expression="SUM(price)"),
+ ],
+ )
+ query_obj: dict[str, Any] = {
+ "granularity": None,
+ "from_dttm": None,
+ "to_dttm": None,
+ "groupby": [],
+ "metrics": [],
+ "columns": [],
+ "is_timeseries": False,
+ "filter": [],
+ **query_obj_fragment,
+ }
+
+ assert (
+ table.has_extra_cache_key_calls(cast(QueryObjectDict, query_obj))
+ == has_extra_cache_keys
+ )
Review Comment:
Nit: `test_extra_cache_keys_in_adhoc_metrics_and_columns` right above also
asserts the resulting `get_extra_cache_keys()`, not only the gate. Worth doing
that here for the positive cases? The gate firing and the key actually
differing are two claims, and only the second one closes the leak.
One thing you would run into: `series_limit_metric` only reaches
`_get_series_orderby` when `series_limit` is truthy and there are series
columns, so those fragments produce an empty key list as written and would need
`series_limit` added to assert a value.
--
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]
