You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "trakshan-mishra (via GitHub)" <gi...@apache.org> on 2026/09/22 07:49:31 UTC
[PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
trakshan-mishra opened a new pull request, #44526:
URL: https://github.com/apache/superset/pull/44526
### SUMMARY
Fixes one symptom of #42926: a chart whose `query_context` was stored by an older version 500s on every non-Explore path.
Those stored payloads carry no `granularity` of their own and no x-axis at all โ the temporal column survives only in `form_data` as `granularity_sqla`. Explore rebuilds the query from `form_data` on every render and so never notices. Every other consumer (alerts & reports, the chart data API, dashboards rendering from the stored context) replays the stored query verbatim, reaches the `not granularity and is_timeseries` check in `models/helpers.py`, and raises.
`_apply_granularity` already has inference for a missing granularity, but it only runs when `should_infer_filter_granularity` passes, and that is gated on `is_adhoc_column(x_axis)`. A legacy chart has no x-axis at all, so it never reaches the inference.
This recovers the column the way Explore effectively does: when there is no x-axis, no `granularity`, and `is_timeseries` is set, take `granularity_sqla` from `form_data`, fall back to the dataset's `main_dttm_col`, and accept the result only if it is a known temporal column.
Two deliberate choices:
- The **absent x-axis** is what distinguishes this legacy shape from a modern chart, so it is tested first. A modern chart always has one.
- `form_data` is preferred over `main_dttm_col`, because what the chart saved should win over the dataset default. A chart explicitly built on a non-default time column keeps that column.
Charts that already carry a `granularity` are left untouched, so this is inert for everything except the broken shape.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable โ backend only, no UI change.
### TESTING INSTRUCTIONS
Three unit tests are included in `tests/unit_tests/common/test_query_context_factory.py`:
```
pytest tests/unit_tests/common/test_query_context_factory.py -q
```
They cover the three branches:
1. `granularity_sqla` in `form_data` is used, and takes precedence over a *different* `main_dttm_col` โ so the test asserts precedence rather than merely that some temporal column was picked.
2. With no `granularity_sqla`, the dataset's `main_dttm_col` is used.
3. A chart that already has a `granularity` is not overwritten.
To reproduce manually: take a chart saved by an older version whose `query_context` has `"granularity": null` and no `x_axis`, with `granularity_sqla` set in `params`, then request it through the chart data API or attach it to an alert. Before this change it returns a 500; after, it renders.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #42926
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
This addresses the symptom I described in [my comment on #42926](https://github.com/apache/superset/issues/42926#issuecomment-5708931274). It fixes the read path at query-build time rather than migrating stored payloads, so it needs no migration and helps charts that are never re-saved. @AryaKetanShCt had mentioned a separate symptom on that issue; this does not overlap with it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44526:
URL: https://github.com/apache/superset/pull/44526#discussion_r4093799202
##########
superset/common/query_context_factory.py:
##########
@@ -275,6 +275,27 @@ def _apply_granularity( # noqa: C901
query_object.granularity = main_dttm_col
return
+ # A ``query_context`` saved by an older version carries no
+ # ``granularity`` of its own and no x-axis at all: its temporal column
+ # survives only in ``form_data`` as ``granularity_sqla``. Explore
+ # rebuilds the query from ``form_data`` on every render and never
+ # notices, but every other consumer replays the stored query verbatim
+ # and fails the ``not granularity and is_timeseries`` check in
+ # ``models.helpers``. The absent x-axis is what distinguishes this shape
+ # from a modern chart, so it is tested first. Recover the column the way
+ # Explore effectively does, preferring what the chart saved over the
+ # dataset default.
+ if (
+ not x_axis
+ and query_object.granularity is None
+ and query_object.is_timeseries
+ ):
+ legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
+ datasource, "main_dttm_col", None
+ )
+ if legacy_granularity in temporal_columns:
+ query_object.granularity = legacy_granularity
Review Comment:
**Suggestion:** When a legacy query has a temporal filter but no `time_range`, this assignment activates later filter removal without adding a replacement, silently dropping the only temporal restriction.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=93569b5aacbc481cb80dbcec038ca113&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=93569b5aacbc481cb80dbcec038ca113&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 288:297
**Comment:**
*Logic Error: When a legacy query has a temporal filter but no `time_range`, this assignment activates later filter removal without adding a replacement, silently dropping the only temporal restriction.
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%2F44526&comment_hash=a82e0a73972e86bd17dbc57cc4dbdaf3d128dac2dc71ec6cb258eddcd3c87de7&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44526&comment_hash=a82e0a73972e86bd17dbc57cc4dbdaf3d128dac2dc71ec6cb258eddcd3c87de7&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "trakshan-mishra (via GitHub)" <gi...@apache.org>.
trakshan-mishra commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5874318132
@rusackas this implements the fallback you described in #42926 (`granularity_sqla`, then `main_dttm_col`), with tests that replay old-style `query_context`s. Ready for review whenever you have time.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5876891686
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5814481340
<!-- Bito Reply -->
The flagged issue is correct. The logic added in `superset/common/query_context_factory.py` correctly identifies the need to recover `granularity` for legacy queries, but it does not account for the scenario where a temporal filter exists in the query object's filters without a corresponding `time_range`. In such cases, the subsequent logic in `_apply_granularity` (which removes filters that match the granularity) may inadvertently remove the only remaining temporal restriction, leaving the query unfiltered.
To resolve this, you should ensure that the filter removal logic only proceeds if the temporal filter is redundant or if a valid replacement is guaranteed. I recommend adding a check to verify if a `time_range` or equivalent temporal constraint exists before allowing the filter removal to proceed.
There are no other comments on this PR to address.
**superset/common/query_context_factory.py**
```
if granularity := query_object.granularity:
# Ensure we do not remove the only temporal filter if no time_range exists
if not query_object.time_range and not self._has_temporal_filter(query_object):
return
filter_to_remove = None
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5820922934
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5773003159
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 4fb3325fc6b7c5d0af4ce5c2ea18e719af7bb430 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab2330ce9dcd10007dba3cb |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44526--superset-docs-preview.netlify.app](https://deploy-preview-44526--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctNCoAgEEDhu8w6yRKKukHHGGemH7A0MVCiu-fuLb73Qoj-DAlmgAZsxIv22uuRWycbUlH3I7Eo8leSnNRWxeMwHqlUzxKcLwvXY0DbG6NJJibutNYjWzRk4fsB) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44526--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "trakshan-mishra (via GitHub)" <gi...@apache.org>.
trakshan-mishra commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5883206037
Closing, since #44460 fixed #42926 and is merged. The one case this PR covered that #44460 does not, an adhoc `granularity_sqla`, is now in #44773.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "trakshan-mishra (via GitHub)" <gi...@apache.org>.
trakshan-mishra closed pull request #44526: fix(query-context): resolve legacy charts' temporal column from granularity_sqla
URL: https://github.com/apache/superset/pull/44526
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5817406289
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44526:
URL: https://github.com/apache/superset/pull/44526#discussion_r4095614567
##########
superset/common/query_context_factory.py:
##########
@@ -275,6 +275,27 @@ def _apply_granularity( # noqa: C901
query_object.granularity = main_dttm_col
return
+ # A ``query_context`` saved by an older version carries no
+ # ``granularity`` of its own and no x-axis at all: its temporal column
+ # survives only in ``form_data`` as ``granularity_sqla``. Explore
+ # rebuilds the query from ``form_data`` on every render and never
+ # notices, but every other consumer replays the stored query verbatim
+ # and fails the ``not granularity and is_timeseries`` check in
+ # ``models.helpers``. The absent x-axis is what distinguishes this shape
+ # from a modern chart, so it is tested first. Recover the column the way
+ # Explore effectively does, preferring what the chart saved over the
+ # dataset default.
+ if (
+ not x_axis
+ and query_object.granularity is None
+ and query_object.is_timeseries
+ ):
+ legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
+ datasource, "main_dttm_col", None
+ )
+ if legacy_granularity in temporal_columns:
+ query_object.granularity = legacy_granularity
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>unhashable dict crash</b></div>
<div id="fix">
`granularity_sqla` can be an adhoc dict (see `migrations/shared/migrate_viz/base.py:135`), so `legacy_granularity in temporal_columns` (a set of str) raises `TypeError: unhashable type: 'dict'` on legacy charts replayed through this path. Guard with `isinstance(legacy_granularity, str)` before the membership test.
</div>
</div>
<small><i>Code Review Run #37a027</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "trakshan-mishra (via GitHub)" <gi...@apache.org>.
trakshan-mishra commented on code in PR #44526:
URL: https://github.com/apache/superset/pull/44526#discussion_r4124545252
##########
superset/common/query_context_factory.py:
##########
@@ -275,6 +275,27 @@ def _apply_granularity( # noqa: C901
query_object.granularity = main_dttm_col
return
+ # A ``query_context`` saved by an older version carries no
+ # ``granularity`` of its own and no x-axis at all: its temporal column
+ # survives only in ``form_data`` as ``granularity_sqla``. Explore
+ # rebuilds the query from ``form_data`` on every render and never
+ # notices, but every other consumer replays the stored query verbatim
+ # and fails the ``not granularity and is_timeseries`` check in
+ # ``models.helpers``. The absent x-axis is what distinguishes this shape
+ # from a modern chart, so it is tested first. Recover the column the way
+ # Explore effectively does, preferring what the chart saved over the
+ # dataset default.
+ if (
+ not x_axis
+ and query_object.granularity is None
+ and query_object.is_timeseries
+ ):
+ legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
+ datasource, "main_dttm_col", None
+ )
+ if legacy_granularity in temporal_columns:
+ query_object.granularity = legacy_granularity
Review Comment:
Fixed in 6216ac9. An adhoc `granularity_sqla` is now reduced to its `sqlExpression` before the membership test against `temporal_columns`, so it resolves instead of raising "unhashable type: 'dict'". Covered by `test_apply_granularity_legacy_adhoc_granularity_sqla`.
##########
superset/common/query_context_factory.py:
##########
@@ -275,6 +275,27 @@ def _apply_granularity( # noqa: C901
query_object.granularity = main_dttm_col
return
+ # A ``query_context`` saved by an older version carries no
+ # ``granularity`` of its own and no x-axis at all: its temporal column
+ # survives only in ``form_data`` as ``granularity_sqla``. Explore
+ # rebuilds the query from ``form_data`` on every render and never
+ # notices, but every other consumer replays the stored query verbatim
+ # and fails the ``not granularity and is_timeseries`` check in
+ # ``models.helpers``. The absent x-axis is what distinguishes this shape
+ # from a modern chart, so it is tested first. Recover the column the way
+ # Explore effectively does, preferring what the chart saved over the
+ # dataset default.
+ if (
+ not x_axis
+ and query_object.granularity is None
+ and query_object.is_timeseries
+ ):
+ legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
+ datasource, "main_dttm_col", None
+ )
+ if legacy_granularity in temporal_columns:
+ query_object.granularity = legacy_granularity
Review Comment:
I checked this against the full query path, and the restriction isn't dropped.
When a query has no `time_range`, `QueryObjectFactory._process_time_range` takes the range from the query's `TEMPORAL_RANGE` filter, so `from_dttm`/`to_dttm` already carry that filter's bounds before `_apply_granularity` runs. Once `granularity` is set, `models/helpers.py` builds the time filter on the granularity column from those same bounds. So the filter removed here is replaced, not lost. This is the same "replace the default temporal filter" behaviour that queries with an explicit granularity already go through.
To pin it, b16ee7f adds `test_apply_granularity_legacy_query_context_keeps_temporal_filter_bounds`. It builds the query object with the real `QueryObjectFactory` (no `time_range`, a `TEMPORAL_RANGE` filter of `2024-01-01 : 2024-02-01`), runs `_apply_granularity` with a legacy `granularity_sqla`, and asserts that the filter is removed while `from_dttm`/`to_dttm` keep those bounds and `granularity` resolves to `ds`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44526:
URL: https://github.com/apache/superset/pull/44526#discussion_r4124547461
##########
superset/common/query_context_factory.py:
##########
@@ -275,6 +275,27 @@ def _apply_granularity( # noqa: C901
query_object.granularity = main_dttm_col
return
+ # A ``query_context`` saved by an older version carries no
+ # ``granularity`` of its own and no x-axis at all: its temporal column
+ # survives only in ``form_data`` as ``granularity_sqla``. Explore
+ # rebuilds the query from ``form_data`` on every render and never
+ # notices, but every other consumer replays the stored query verbatim
+ # and fails the ``not granularity and is_timeseries`` check in
+ # ``models.helpers``. The absent x-axis is what distinguishes this shape
+ # from a modern chart, so it is tested first. Recover the column the way
+ # Explore effectively does, preferring what the chart saved over the
+ # dataset default.
+ if (
+ not x_axis
+ and query_object.granularity is None
+ and query_object.is_timeseries
+ ):
+ legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
+ datasource, "main_dttm_col", None
+ )
+ if legacy_granularity in temporal_columns:
+ query_object.granularity = legacy_granularity
Review Comment:
<!-- Bito Reply -->
The suggestion to guard the membership test with `isinstance(legacy_granularity, str)` is appropriate. It prevents a `TypeError` when `granularity_sqla` is an adhoc dictionary, ensuring the code safely handles legacy chart configurations.
**superset/common/query_context_factory.py**
```
legacy_granularity = (form_data or {}).get("granularity_sqla") or getattr(
datasource, "main_dttm_col", None
)
if isinstance(legacy_granularity, str) and legacy_granularity in temporal_columns:
query_object.granularity = legacy_granularity
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5772971890
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(query-context): resolve legacy charts' temporal column from granularity_sqla [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44526:
URL: https://github.com/apache/superset/pull/44526#issuecomment-5773572840
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44526?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.97%. Comparing base ([`4c77b0b`](https://app.codecov.io/gh/apache/superset/commit/4c77b0b924a9bd770110915eacd68f5619e8241d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4fb3325`](https://app.codecov.io/gh/apache/superset/commit/4fb3325fc6b7c5d0af4ce5c2ea18e719af7bb430?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 135 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44526?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/common/query\_context\_factory.py](https://app.codecov.io/gh/apache/superset/pull/44526?src=pr&el=tree&filepath=superset%2Fcommon%2Fquery_context_factory.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfZmFjdG9yeS5weQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (4c77b0b) and HEAD (4fb3325). Click for more details.
>
> <details><summary>HEAD has 125 uploads less than BASE</summary>
>
>| Flag | BASE (4c77b0b) | HEAD (4fb3325) |
>|------|------|------|
>|hive|18|1|
>|python|109|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44526 +/- ##
===========================================
- Coverage 80.40% 55.97% -24.44%
===========================================
Files 2932 2942 +10
Lines 174525 176773 +2248
Branches 40548 40964 +416
===========================================
- Hits 140319 98940 -41379
- Misses 31537 77317 +45780
+ Partials 2669 516 -2153
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.93% <0.00%> (-0.23%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.93% <0.00%> (-47.82%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44526?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org