gabotorresruiz commented on code in PR #45431:
URL: https://github.com/apache/superset/pull/45431#discussion_r4362665776
##########
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:
Agreed, and the trade-off is even cheaper than you framed it.
`get_extra_cache_keys` only collects `sqla_query.extra_cache_keys`
(`models.py:2733`), which the template processor fills from what it actually
rendered, so an over-firing gate costs the render and cannot invent a key. The
bare `series_limit_metric` comes out `gate=True` with `keys=[]` for both
principals, so the key itself does not move.
`test_extra_cache_keys_differ_once_the_gate_fires` is the right shape, and
each of the three new conditions has a named case that goes red when you revert
that condition on its own, including the raw-expression default you kept in the
adhoc lookup.
--
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]
