codeant-ai-for-open-source[bot] commented on code in PR #42052:
URL: https://github.com/apache/superset/pull/42052#discussion_r3588838875
##########
superset/common/query_context_factory.py:
##########
@@ -245,6 +245,20 @@ def _apply_granularity( # noqa: C901
}
x_axis = form_data and form_data.get("x_axis")
+ should_infer_filter_granularity = (
+ is_adhoc_column(x_axis) # type: ignore
+ and query_object.granularity is None
+ and bool(query_object.from_dttm or query_object.to_dttm)
+ and (main_dttm_col := getattr(datasource, "main_dttm_col", None))
+ in temporal_columns
+ )
Review Comment:
**Suggestion:** The inference guard uses `from_dttm`/`to_dttm` presence as a
proxy for dashboard time filtering, but those bounds are also populated from
existing temporal filters in `QueryObjectFactory._process_time_range`. This
means charts with an adhoc x-axis and an explicit temporal filter on a
different datetime column can now get an unintended extra filter on the dataset
main datetime column, changing query semantics and potentially dropping valid
rows. Restrict inference to cases where the active temporal filter target is
actually the dataset main datetime column (or when no other temporal filters
are present). [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart data queries add unintended main datetime filters.
- ⚠️ Dashboard charts may silently drop valid filtered rows.
- ⚠️ Async chart data tasks share same incorrect filtering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A client submits a chart data request to the Chart Data API, which is
handled in
`superset/charts/data/api.py:60-80` (`ChartsDataApi.data`). The request body
contains a
`query_context` JSON matching `ChartDataQueryContextSchema` with:
- an adhoc SQL expression as `x_axis` in the query object,
- an explicit `TEMPORAL_RANGE` filter on a datetime column different from
the dataset
`main_dttm_col`,
- no explicit `granularity` and no top-level `time_range`.
2. The request JSON is deserialized by `ChartDataQueryContextSchema` in
`superset/charts/schemas.py:13-40`. Its `post_load` hook
`make_query_context()` (lines
37-40) calls `self.get_query_context_factory().create(**data)`, which
instantiates
`QueryContextFactory` and invokes `QueryContextFactory.create` in
`superset/common/query_context_factory.py:45-87`. That method:
- converts the datasource to an `Explorable`,
- iterates `queries` and, for each query dict, calls
`self._query_object_factory.create(...)` from `QueryObjectFactory` in
`superset/common/query_object_factory.py:13-56`.
3. Inside `QueryObjectFactory.create`
(`superset/common/query_object_factory.py:41-56`),
`_process_time_range()` is called (lines 41-43) with the query’s
`time_range`, `filters`,
and `columns`. `_process_time_range` at lines 95-119:
- detects `time_range is None` and sets it to `NO_TIME_RANGE`,
- scans `filters` for any with `op == FilterOperator.TEMPORAL_RANGE`
(lines 102-106),
- if temporal filters exist, picks either the one whose `col` matches the
x-axis label
(`get_x_axis_label(columns)`) or the first temporal filter (lines
111-118),
- sets `time_range` to that filter’s `val` string.
`get_since_until_from_time_range` is then called (lines 44-45) to derive
`from_dttm`
and `to_dttm` from this temporal filter, and these values are stored on
the
`QueryObject` (`kwargs["from_dttm"]` and `kwargs["to_dttm"]` at lines
47-48),
regardless of which datetime column the original filter targeted.
4. Back in `QueryContextFactory.create`, the constructed `QueryObject` is
passed into
`_process_query_object` (`superset/common/query_context_factory.py:77-88`),
which calls
`_apply_granularity` (lines 24-27). In `_apply_granularity` (lines 136-155),
the new
guard:
- builds `temporal_columns` from all datasource columns with `is_dttm`
true (lines
142-146),
- reads `x_axis` from `form_data` (line 147),
- computes `should_infer_filter_granularity` at lines 149-155 as:
`is_adhoc_column(x_axis)`
and `query_object.granularity is None`
and `bool(query_object.from_dttm or query_object.to_dttm)`
and `(main_dttm_col := getattr(datasource, "main_dttm_col", None)) in
temporal_columns`.
In the scenario above, this condition becomes true even though the active
temporal
filter that populated `from_dttm/to_dttm` is on a different datetime
column, not
`main_dttm_col`. The code then sets `query_object.granularity =
main_dttm_col` and
returns early at lines 156-160, bypassing the later block (lines 259-65)
that
selectively removes a temporal filter to avoid duplication.
Downstream, when the datasource executes `datasource.query(query_obj)`
(see
`superset/viz.py:20-26`), the query builder uses
`QueryObject.granularity` together
with `from_dttm/to_dttm` to add a default time predicate on
`main_dttm_col`. Because
`_apply_granularity` returned early, the original `TEMPORAL_RANGE` filter
on the other
datetime column remains in `query_object.filter`. The resulting SQL now
includes both:
- the user’s explicit temporal filter on the non-main datetime column, and
- an additional inferred time filter on the dataset `main_dttm_col` with
the same
bounds.
This unintended second filter on a different column alters query
semantics and can drop
rows that satisfy the original filter but fall outside the main datetime
range.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=42d6954aca9a4f51be742c86dfc900a0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=42d6954aca9a4f51be742c86dfc900a0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/common/query_context_factory.py
**Line:** 248:254
**Comment:**
*Incorrect Condition Logic: The inference guard uses
`from_dttm`/`to_dttm` presence as a proxy for dashboard time filtering, but
those bounds are also populated from existing temporal filters in
`QueryObjectFactory._process_time_range`. This means charts with an adhoc
x-axis and an explicit temporal filter on a different datetime column can now
get an unintended extra filter on the dataset main datetime column, changing
query semantics and potentially dropping valid rows. Restrict inference to
cases where the active temporal filter target is actually the dataset main
datetime column (or when no other temporal filters are present).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42052&comment_hash=38f1679f2d3b204ef4cdd57c85faf74af6817b192f4f3538f77dde587736f523&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42052&comment_hash=38f1679f2d3b204ef4cdd57c85faf74af6817b192f4f3538f77dde587736f523&reaction=dislike'>👎</a>
--
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]
