codeant-ai-for-open-source[bot] commented on code in PR #41290:
URL: https://github.com/apache/superset/pull/41290#discussion_r3454823797
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -332,6 +332,18 @@ def apply_dashboard_filter_context(
for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS:
if key in extra_form_data:
extras[key] = extra_form_data[key]
+
+ # EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS is originally used with
form_data objects,
+ # not query_context objects. form_data objects expect time_grain_sqla
as a
+ # top-level key, but query_context objects expect it as an extra key.
+ if custom_time_grain := extra_form_data.get("time_grain_sqla"):
+ extras["time_grain_sqla"] = custom_time_grain
Review Comment:
**Suggestion:** The time-grain override is gated by a truthiness check, so a
provided falsy value (for example an explicit empty/null-like value used to
clear an existing grain) is ignored instead of being applied. This leaves stale
chart-level grain values in `extras`/columns and makes dashboard overrides
non-deterministic. Check for key presence (or `is not None`) rather than
truthiness when deciding whether to apply the override. [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard time-grain clear operations silently fail.
- ⚠️ Chart and dashboard grains become inconsistent.
- ⚠️ Future falsy time-grain overrides behave non-deterministically.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The chart data GET endpoint `/api/v1/chart/<pk>/data` builds `json_body`
and, when
`filters_dashboard_id` is present, calls `get_dashboard_filter_context()`
and then
`apply_dashboard_filter_context(json_body, efd)` at
`superset/charts/data/api.py:14-29`.
2. `get_dashboard_filter_context()` in
`superset/charts/data/dashboard_filter_context.py:18-47` merges native
filters'
`extra_form_data` into a single dict; if a time-grain native filter is
configured to clear
the grain it can yield `extra_form_data={"time_grain_sqla": ""}` or
`{"time_grain_sqla":
None}`.
3. `apply_dashboard_filter_context()` at
`superset/charts/data/dashboard_filter_context.py:64-86` executes `if
custom_time_grain :=
extra_form_data.get("time_grain_sqla"):` (line 339 in the PR hunk); for a
falsy override
like `""` or `None`, `custom_time_grain` is falsy so the block is skipped and
`extras["time_grain_sqla"]` is left at the prior chart-level value.
4. The resulting `query_context` (built later via
`_create_query_context_from_form(json_body)` in
`superset/charts/data/api.py:38-40`) still
carries the old `extras["time_grain_sqla"]`, so any backend consumer (e.g.
`QueryObjectHelper.get_time_grain()` in `superset/models/helpers.py:2011-22`
for charts
without adhoc x-axis) sees the stale grain and the dashboard attempt to
clear or override
the grain is ignored.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5edbda92143e497cb54c9c4be777d6e3&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=5edbda92143e497cb54c9c4be777d6e3&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/charts/data/dashboard_filter_context.py
**Line:** 339:340
**Comment:**
*Incorrect Condition Logic: The time-grain override is gated by a
truthiness check, so a provided falsy value (for example an explicit
empty/null-like value used to clear an existing grain) is ignored instead of
being applied. This leaves stale chart-level grain values in `extras`/columns
and makes dashboard overrides non-deterministic. Check for key presence (or `is
not None`) rather than truthiness when deciding whether to apply the override.
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%2F41290&comment_hash=019e7b85535cf344d531642ace43fec1e36efc151ec1440ab2b1baba8b1b64b2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41290&comment_hash=019e7b85535cf344d531642ace43fec1e36efc151ec1440ab2b1baba8b1b64b2&reaction=dislike'>👎</a>
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -332,6 +332,18 @@ def apply_dashboard_filter_context(
for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS:
if key in extra_form_data:
extras[key] = extra_form_data[key]
+
+ # EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS is originally used with
form_data objects,
+ # not query_context objects. form_data objects expect time_grain_sqla
as a
+ # top-level key, but query_context objects expect it as an extra key.
+ if custom_time_grain := extra_form_data.get("time_grain_sqla"):
+ extras["time_grain_sqla"] = custom_time_grain
+ # Inject ``filter_timegrain`` into X-Axis column
+ for column in query.get("columns") or []:
+ if isinstance(column, dict) and column.get("columnType") ==
"BASE_AXIS":
+ column["timeGrain"] = custom_time_grain
+ break
Review Comment:
**Suggestion:** This logic updates only columns tagged as `BASE_AXIS`, but
backend time-grain resolution reads the first adhoc column directly, not by
`columnType`. If the first column is not the one marked `BASE_AXIS`, the
dashboard grain can still be ignored at query time. Update the column that
backend time-grain resolution actually inspects (or align both behaviors) to
avoid this contract mismatch. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard grain ignored for time-comparison charts with extra columns.
- ⚠️ Time offsets computed using stale chart-level grain.
- ⚠️ Users see inconsistent grains between main and offset series.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A line chart with an adhoc X-axis and an additional adhoc dimension is
built;
`build_query_context()` and `normalize_time_column()` in
`superset/migrations/shared/migrate_viz/query_functions.py:15-52` mark the
x-axis column
as `columnType="BASE_AXIS"` at its existing index `axis_idx`, but they do
not move it to
position 0 in `query["columns"]`.
2. A dashboard-level time-grain native filter is applied and merged via
`get_dashboard_filter_context()`
(`superset/charts/data/dashboard_filter_context.py:18-47`), producing
`extra_form_data={"time_grain_sqla": "P1Y"}` that is then passed into
`apply_dashboard_filter_context(json_body, efd)` from
`superset/charts/data/api.py:14-29`.
3. Inside `apply_dashboard_filter_context()` at
`superset/charts/data/dashboard_filter_context.py:72-86`, the code loops over
`query["columns"]` and updates only the column where
`column.get("columnType") ==
"BASE_AXIS"`: `column["timeGrain"] = custom_time_grain` (lines 342-345),
leaving any
earlier adhoc column (`query["columns"][0]`) unchanged if it is not marked
`BASE_AXIS`.
4. When the time comparison feature runs, `processing_time_offsets()` in
`superset/models/helpers.py:4-49` calls `time_grain =
self.get_time_grain(query_object)`,
and `get_time_grain()` at `superset/models/helpers.py:2011-22` returns
`query_object.columns[0].get("timeGrain")` without checking `columnType`; if
the base axis
is not at index 0, it reads an unmodified or `None` `timeGrain` and never
falls back to
`extras["time_grain_sqla"]`, so time comparison uses the stale chart grain
and ignores the
dashboard grain override.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a05a553585fe422fbce02457a1945953&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=a05a553585fe422fbce02457a1945953&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/charts/data/dashboard_filter_context.py
**Line:** 342:345
**Comment:**
*Api Mismatch: This logic updates only columns tagged as `BASE_AXIS`,
but backend time-grain resolution reads the first adhoc column directly, not by
`columnType`. If the first column is not the one marked `BASE_AXIS`, the
dashboard grain can still be ignored at query time. Update the column that
backend time-grain resolution actually inspects (or align both behaviors) to
avoid this contract mismatch.
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%2F41290&comment_hash=a03c3bde90e365328874091dff3707cafb1e2aa5037d41caa5dd1edfefe31c9b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41290&comment_hash=a03c3bde90e365328874091dff3707cafb1e2aa5037d41caa5dd1edfefe31c9b&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]
