SEPURI-SAI-KRISHNA commented on code in PR #45431:
URL: https://github.com/apache/superset/pull/45431#discussion_r4361493602
##########
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:
Done, and the trap you flagged is real: `series_limit_metric` on its own
gives `gate=True` with `keys=[]`, since both `_get_series_orderby` call sites
sit behind `if series_limit and groupby_series_columns` (`helpers.py:5743`).
`test_extra_cache_keys_differ_once_the_gate_fires` asserts
`get_extra_cache_keys` for the positive cases and shapes each query so the
field really is rendered. The series-limit case carries `series_limit`, a
groupby and `is_timeseries`.
I left the gate firing for a `series_limit_metric` with no `series_limit`.
Over-firing costs one render that finds no keys, under-firing costs a wrong
cache hit.
Rebased onto master as well, which clears the two red unit tests. Thanks for
tracking those down.
##########
superset/connectors/sqla/models.py:
##########
@@ -2593,26 +2593,62 @@ 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")
- ):
+ calculated_columns: dict[str, Any] = {
+ c.column_name: c.expression for c in self.columns if c.expression
+ }
+ 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):
+ return metric.get("sqlExpression")
+ if isinstance(metric, str):
+ return metrics_by_name.get(metric)
+ 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`` is an adhoc or calculated column
+ for order_by in query_obj.get("orderby") or []:
+ entry = order_by[0] if isinstance(order_by, (list, tuple)) else
order_by
+ if isinstance(entry, dict):
+ if sql := entry.get("sqlExpression"):
templatable_statements.append(sql)
- elif isinstance(metric, str) and metric in metrics_by_name:
- templatable_statements.append(metrics_by_name[metric])
+ elif 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)
Review Comment:
Addressed. `metric_expression` resolves a SIMPLE adhoc metric through
`column.column_name` against the dataset's calculated columns, which is what
`adhoc_metric_to_sqla` wraps its aggregate around. The `SIMPLE_JINJA_METRIC`
cases cover it in `metrics`, `orderby` and `series_limit_metric`.
--
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]
