gabotorresruiz commented on code in PR #43176:
URL: https://github.com/apache/superset/pull/43176#discussion_r4030438675
##########
superset/charts/data/form_data.py:
##########
@@ -31,33 +33,132 @@ def set_form_data(form_data: dict[str, Any]) -> None:
g.form_data = form_data
+def _as_form_data_dict(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _as_query_list(value: Any) -> list[Any]:
+ if isinstance(value, (list, tuple)):
+ return list(value)
+ return []
+
+
+def _filter_op(op: Any) -> str | None:
+ if isinstance(op, FilterOperator):
+ return op.value
+ return op if isinstance(op, str) else None
+
+
+def _raw_query_dicts(query_context: QueryContext) -> list[Any]:
+ cache_values = getattr(query_context, "cache_values", None)
+ if not isinstance(cache_values, dict):
+ return []
+ return _as_query_list(cache_values.get("queries"))
+
+
+def _time_range_from_filters(filters: Any, time_column: Any = None) -> str |
None:
+ """Read a time range for Jinja get_time_filter().
+
+ Prefer a TEMPORAL_RANGE comparator (datasets and two-sided views).
+ Semantic views rewrite one-sided ranges to ``>=`` / ``<`` on the
+ granularity column and emit no TEMPORAL_RANGE, so reconstruct those
+ in the ``since : until`` grammar get_time_filter() already parses.
+ """
+ if not isinstance(filters, list):
+ return None
+
+ since: str | None = None
+ until: str | None = None
+ for flt in filters:
+ if not isinstance(flt, dict):
+ continue
+ op = _filter_op(flt.get("op"))
+ val = flt.get("val")
+ if op == FilterOperator.TEMPORAL_RANGE.value and isinstance(val, str):
+ return val
+ if time_column is None or flt.get("col") != time_column:
+ continue
+ if not isinstance(val, str):
+ continue
+ if op == FilterOperator.GREATER_THAN_OR_EQUALS.value and since is None:
+ since = val
+ elif op == FilterOperator.LESS_THAN.value and until is None:
+ until = val
+
+ if since is None and until is None:
+ return None
+ return f"{since or ''} : {until or ''}"
+
+
+def _hoisted_time_range(
+ query: QueryObject,
+ filters: Any,
+ raw_query: Any = None,
+) -> str | None:
+ if (time_range := getattr(query, "time_range", None)) is not None:
+ return time_range
+ granularity = getattr(query, "granularity", None)
+ if hoisted := _time_range_from_filters(filters, granularity):
+ return hoisted
+ raw = raw_query if isinstance(raw_query, dict) else {}
+ # QueryContextFactory._apply_granularity deletes TEMPORAL_RANGE on the
+ # granularity column before this helper runs. The raw pre-processing
+ # dict in cache_values still has it.
+ return _time_range_from_filters(
+ raw.get("filters"), granularity or raw.get("granularity")
+ )
+
+
def _serialize_query(
query: QueryObject,
form_data: dict[str, Any],
-) -> dict[str, Any]:
- """Serialize query fields consumed by the Jinja form-data fallback."""
- query_data = dict(query.to_dict())
- query_data["filters"] = query.filter
- if query.time_range is not None:
- query_data["time_range"] = query.time_range
+ raw_query: Any = None,
+) -> dict[str, Any] | None:
+ """Serialize query fields consumed by the Jinja form-data fallback.
+
+ Incomplete stubs (unit-test doubles without ``to_dict``) are skipped so
+ callers can still publish datasource context for Jinja without requiring a
+ full ``QueryObject``.
+ """
+ to_dict = getattr(query, "to_dict", None)
+ if not callable(to_dict):
+ return None
+
+ query_data = dict(to_dict())
+ filters = getattr(query, "filter", None)
+ query_data["filters"] = filters
+ if hoisted := _hoisted_time_range(query, filters, raw_query):
Review Comment:
This block worries me a bit. `_hoisted_time_range` sits inside
`_serialize_query`, so it fires for every `set_query_context_form_data` caller,
including `superset/tasks/async_queries.py:298`, which is the async chart-data
path and not an MCP one. The helper's contract is "Expose a
programmatically-created query like a chart data API request"
(`form_data.py:149`), and that parity is what it now breaks.
I verified it at `ddc5617`. Same ECharts timeseries query (string `x_axis`,
no `granularity`, range carried only as a `TEMPORAL_RANGE` filter), one virtual
dataset template, both paths:
```sql
-- chart-data API request body
SELECT * FROM events WHERE region = 'North' AND range = 'No filter'
-- g.form_data path (async worker, MCP tools)
SELECT * FROM events WHERE region = 'North' AND range = 'Last week'
```
`filter_values` agrees, which is this PR working. `get_time_filter()` does
not. Both paths returned `No filter` at `b98f43b` and on master, so the
divergence is new in `1468a17` / `27ded6a`.
The `>=` / `<` reconstruction has a second edge. `get_table` sets
`granularity` from `time_column` even when no `time_range` is given
(`get_table.py:112` and `:326`), so `_apply_granularity` has no
`TEMPORAL_RANGE` to strip and removes nothing, and `GetTableFilter.op` is a
free string (`semantic_layer/schemas.py:140`). So `get_table(time_column="ds",
filters=[{"col": "ds", "op": ">=", "val": "Q1"}])` makes `get_time_filter()`
raise `TimeRangeParseFailError: Cannot parse time string [Q1]` where master
renders `No filter`. A parseable value is wrong in a quieter way: it turns a
plain comparison filter into the reported time range.
Could we scope the hoist to the callers that need it?
`execute_tabular_query` already holds the raw pre-factory dict, so it can pass
the range explicitly and the shared helper goes back to being a pure serializer:
```python
# superset/common/tabular_query.py
set_query_context_form_data(
query_context,
datasource_id,
datasource_type,
time_range=_time_range_from_filters(
query_dict.get("filters"), query_dict.get("granularity")
),
)
```
with a `time_range: str | None = None` argument threaded down to
`_serialize_query`, and `_hoisted_time_range` / `_raw_query_dicts` dropped from
the shared path. Chart and async callers go back to byte identical with master,
`QueryObject.time_range` stays unset so your rollover fix is untouched, and the
existing hoist tests move to the `execute_tabular_query` call shape.
For coverage: a chart-shaped case in
`tests/unit_tests/charts/data/form_data_test.py` asserting
`get_time_filter().time_range == NO_TIME_RANGE` when the query carries only a
`TEMPORAL_RANGE` filter and no explicit range, plus a `get_table` case with
`time_column` set and no `time_range`. Both fail on this head today.
Happy to dig in with you on this one if it helps.
--
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]
