fix(explore): keep x-axis label when overriding Time Column with time comparison - #42875
fix(explore): keep x-axis label when overriding Time Column with time comparison#42875yousoph wants to merge 2 commits into
Conversation
Code Review Agent Run #1293f3Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| """Build the reporter's dataset (monthly wedding/purchase dates).""" | ||
| fd, path = tempfile.mkstemp(suffix=".db") | ||
| os.close(fd) | ||
| uri = f"sqlite:///{path}" |
There was a problem hiding this comment.
Suggestion: Each invocation creates a persistent named temporary database file, but the path is never removed after the test. Since both tests construct a dataset and repeated test runs reuse this helper, these files accumulate in the system temporary directory; add teardown or use a temporary-directory context that removes the database after use. [resource leak]
Severity Level: Minor 🧹
- ⚠️ Repeated test runs accumulate temporary SQLite files.
- ⚠️ Long-lived CI workers may experience temporary-disk growth.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_time_column_offset_repro.py
**Line:** 59:62
**Comment:**
*Resource Leak: Each invocation creates a persistent named temporary database file, but the path is never removed after the test. Since both tests construct a dataset and repeated test runs reuse this helper, these files accumulate in the system temporary directory; add teardown or use a temporary-directory context that removes the database after use.
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 fixThere was a problem hiding this comment.
Good catch — fixed. The helper now takes a path from pytest's tmp_path fixture, which is auto-removed after each test, so no temporary DB files accumulate.
| def _run(query_object: QueryObject) -> pd.DataFrame: | ||
| table = cast(SqlaTable, query_object.datasource) | ||
| result = table.get_query_result(query_object) | ||
| return query_object.exec_post_processing(result.df) |
There was a problem hiding this comment.
Suggestion: SqlaTable.get_query_result already performs time-offset processing and executes query_object.exec_post_processing before returning. Calling exec_post_processing again applies the pivot and flatten operations twice, so this regression test does not exercise the production result contract and can produce misleading results or fail when post-processing operations are not idempotent. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Regression assertions exercise a non-production pipeline.
- ⚠️ Non-idempotent post-processing can create false failures.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/common/test_time_column_offset_repro.py
**Line:** 166:169
**Comment:**
*Incomplete Implementation: `SqlaTable.get_query_result` already performs time-offset processing and executes `query_object.exec_post_processing` before returning. Calling `exec_post_processing` again applies the pivot and flatten operations twice, so this regression test does not exercise the production result contract and can produce misleading results or fail when post-processing operations are not idempotent.
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 fixThere was a problem hiding this comment.
Correct — get_query_result already runs the offset join and exec_post_processing. Removed the second call; _run now returns get_query_result(...).df directly, so the test exercises the production result contract exactly once.
… comparison (SC-111332) Overriding a chart's Time Column on a dashboard (via the native "Time Column" filter or the Display Controls dropdown — both emit `granularity_sqla` through `extra_form_data`) funnels through `QueryContextFactory._apply_granularity`. That method re-points the BASE_AXIS x-axis at the overridden column, but it also renamed the column's `label` to the overridden column name. The offset join in `processing_time_offsets`, the post-processing pivot `index`, and the frontend (`getXAxisLabel`) all reference the x-axis by its original saved label. Renaming the label desynchronized those consumers from the label the saved chart still advertises. With a Time Comparison offset in play, the result was keyed under a label nothing else recognized, collapsing the series into a single data point. Fix: for an adhoc (dict) x-axis, only swap the underlying `sqlExpression` to the overridden column and keep the original `label`, so the join, the pivot and the frontend keep matching. The bare-string x-axis path (which has no distinct label) still replaces the column wholesale and realigns the pivot `index`. Adds a regression test that drives the full offset + pivot pipeline and asserts the overridden Time Column plots across the range (multi-point) under the original x-axis label instead of collapsing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f8ac93d to
76df474
Compare
Code Review Agent Run #4bf88aActionable 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 |
sadpandajoe
added
the
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 5272d59 🚦 building
github-actions
Bot
added
🎪 5272d59 🚦 deploying
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42875 +/- ##
==========================================
+ Coverage 57.10% 66.37% +9.27%
==========================================
Files 2857 2856 -1
Lines 161125 161106 -19
Branches 37060 37051 -9
==========================================
+ Hits 92014 106941 +14927
+ Misses 68259 52146 -16113
- Partials 852 2019 +1167
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
🎪 5272d59 🚦 running
github-actions
Bot
added
🎪 5272d59 🚦 running
|
🎪 Showtime deployed environment on GHA for 5272d59 • Environment: http://18.237.97.97:8080 (admin/admin) |
2 participants
SUMMARY
Fixes a bug where a chart with Time Comparison (a time offset such as
1 year ago) collapses into a single data point when its Time Column is overridden on a dashboard to a non-default temporal column. Reported by The Knot (Preset SC-111332).Both dashboard entry points for the Time Column — the native "Time Column" filter and the Display Controls dropdown — emit
granularity_sqlathroughextra_form_data, which maps togranularityand converges on a single backend method:QueryContextFactory._apply_granularity.For an adhoc (BASE_AXIS) x-axis, that method re-points the x-axis at the overridden column but also renamed the column's
labelto the overridden column name:The offset join in
processing_time_offsets, the post-processing pivotindex, and the frontend series extraction (getXAxisLabel(rawFormData)inTimeseries/transformProps.ts) all reference the x-axis by its original saved label. Renaming the label desynchronizes those consumers from the label the saved chart still advertises. With a Time Comparison offset in play, the result is keyed under a label nothing downstream recognizes, so ECharts treats the temporal column as a numeric series and the x-axis collapses to a single point (the reported symptom: a lone point with the temporal column in the legend and a y-value in the trillions — the timestamp in milliseconds).Fix: for an adhoc x-axis, only swap the underlying
sqlExpressionto the overridden column and keep the originallabel, so the join, the pivot and the frontend keep referencing the same label. The bare-string x-axis path (which has no distinct label) still replaces the column wholesale and realigns the pivotindex.Because both override paths converge on
_apply_granularity, this fixes the issue from both the Filters panel and the Display Controls.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: overriding the Time Column with a Time Comparison configured collapses the series into a single point.
After: the series plots across the full range under the original x-axis label, with the offset (
… 1 year ago) populated.TESTING INSTRUCTIONS
wedding_date, metric = revenue, date range2025-01-01-> today, and Time Comparison1 year ago. Save to a dashboard.purchase_date.purchase_dateacross the range instead of collapsing to a single point.Automated coverage:
pytest tests/unit_tests/common/test_time_column_offset_repro.pydrives the full offset + pivot pipeline and asserts the overridden Time Column plots multiple points under the original x-axis label.tests/unit_tests/common/test_query_context_factory.py::test_apply_granularity_with_x_axis_dictasserts the label is preserved.ADDITIONAL INFORMATION
🤖 Generated with Claude Code