fix(query): align granularity with temporal x-axis and preserve secondary TEMPORAL_RANGE filters#39683
fix(query): align granularity with temporal x-axis and preserve secondary TEMPORAL_RANGE filters#39683ishimasar wants to merge 8 commits into
Conversation
…dary dttm filters X軸が日時のとき granularity を x_axis に固定し、別日時列の TEMPORAL_RANGE を消さない/time_range の上書き対象を主軸列に限定。time_range 推定はX軸に一致する TEMPORAL_RANGE を優先
Document why we prefer TEMPORAL_RANGE on BASE_AXIS and avoid falling back to temporal_flt[0] when the x-axis is known but has no matching filter (prevents another datetime column from driving from_dttm/to_dttm). Add unit tests for: no fallback when only a secondary column has TEMPORAL_RANGE; legacy use of the first temporal when no BASE_AXIS; choosing the x-axis filter when multiple temporals exist; adhoc filter column dicts matched via sqlExpression.
Code Review Agent Run #5e3aa5Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
change:backend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
Thanks for the PR, @ishimasar — the problem statement and approach look well-thought-out. This directly addresses the gaps we discussed on #39681 [1]. A few observations and things to watch for: Adhoc column normalization — this is the most impactful part of the fix. The current Fallback behavior change — returning Secondary filter preservation in Backward compatibility — since this changes observable behavior (granularity locking, no fallback, preserved secondary filters), it might be worth adding a note in the PR description about what dashboards might see differently after the fix, even if the new behavior is strictly more correct. Overall the direction looks solid and complements the prior work in #36309, #36985, and #38978 well. Looking forward to reviewing the code changes. To reply, just mention @dosu. Share context across your team and agents. Try Dosu.
|
|
Thanks @ishimasar, I've reviewed the corrected commit ( 1. Duplicate helpers — consider consolidating. 2. Adhoc column check inconsistency. In 3. 4. 5. Test coverage. The four new test cases cover the key scenarios well (no fallback when x-axis has no matching temporal filter, legacy fallback when no x-axis, preference for x-axis match, and adhoc dict matching). The bot flagged a missing docstring on Nice work overall — the most impactful suggestion would be consolidating the duplicate helpers and using To reply, just mention @dosu. Share context across your team and agents. Try Dosu.
|
Code Review Agent Run #72ca74Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Add get_query_object_filter_column_name in superset/utils/core.py and use it from QueryObjectFactory and QueryContextFactory instead of duplicate helpers. Clarify unit test docstrings for _process_time_range scenarios. Made-with: Cursor
|
@dosu Thanks for the detailed review. Duplicate helpers: Consolidated into get_query_object_filter_column_name in superset/utils/core.py, imported from both query_object_factory and query_context_factory. Pushed in the latest commit on the PR branch. |
|
Thanks @ishimasar — the latest commit (
Everything looks good to me. The changes are well-scoped and the test coverage hits the key scenarios (no wrong fallback, legacy fallback, x-axis preference, adhoc dict matching). To reply, just mention @dosu. Share context across your team and agents. Try Dosu.
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #813f98Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@dosu |
|
The merge decision follows the standard Apache Superset contribution process:
So the next steps for this PR would be ensuring CI is green, and then waiting for a committer to review and approve. If the PR stays inactive for more than 30 days without review, it may be closed automatically [5], so it can help to tag a relevant committer or mention it in the dev mailing list or Slack to get attention. To reply, just mention @dosu. Docs are dead. Just use Dosu.
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Looking good to me! Kicking off CI, and planning to spin up a test env. Upon review it looks like the NO_TIME_RANGE fallback change is a subtle breaking behavior change. When BASE_AXIS is set but no matching filter exists, the old code would silently fall back to the first temporal filter's range. The new code returns NO_TIME_RANGE, resulting in an unbounded query. It's arguably more correct, but it will visibly affect existing dashboards where the old behavior happened to work. There should at minimum be a note in the description, and ideally this should be called out in an UPDATING.md entry. Adding @sadpandajoe, since this is something we'll definitely want to test thoroughly. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39683 +/- ##
==========================================
- Coverage 63.88% 63.34% -0.55%
==========================================
Files 2583 2583
Lines 136604 136603 -1
Branches 31502 31500 -2
==========================================
- Hits 87276 86536 -740
- Misses 47812 48549 +737
- Partials 1516 1518 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
SUMMARY
Fixes incorrect
time_range/ granularity alignment when a dataset has multiple datetime columns and native filters attachTEMPORAL_RANGEto a non–x-axis column. Also improves adhoc column (sqlExpression/label) matching and avoids stripping legitimate secondary temporal filters.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
event_a,event_b).event_a.event_bonly.ADDITIONAL INFORMATION
Related context
Complements recent work around temporal column resolution (e.g. timeshift / native-filters / MCP chart generation). This PR specifically tightens:
_process_time_range(no wrong fallback to the first temporal filter when the axis is known),QueryContextFactory,time_rangepropagation that preserves unrelatedTEMPORAL_RANGEclauses,Refs discussion with @dosu on the issue thread.
I manually checked the relevant chart before merging.