You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Anikesh0001 (via GitHub)" <gi...@apache.org> on 2026/10/20 10:43:40 UTC
[PR] fix(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Anikesh0001 opened a new pull request, #45519:
URL: https://github.com/apache/superset/pull/45519
### SUMMARY
Fixes #45474.
Since #44628, `getGrainBarMaxWidth` caps the bar width at one time-grain bucket for every grain in `TIMEGRAIN_TO_TIMESTAMP`. That cap was meant for sparse **sub-daily** data, as the comment in `Timeseries/transformers.ts` says ("For a sub-daily time grain, transformProps.ts overrides this…"). For Day and coarser grains it misbehaves when the data is spaced further apart than the grain, which is the common case of the default Day grain on pre-aggregated monthly or yearly rows. One day is then a sliver of the axis, and the bars are drawn as hairlines.
This change follows the fix proposed in the issue:
- `getGrainBarMaxWidth` returns `undefined` when the grain is a day or longer (`grainMs >= ONE_DAY_MS`).
- Both call sites (`Timeseries/transformProps.ts` and `MixedTimeseries/transformProps.ts`) already keep the default `barMaxWidth: 100` in that case, so ECharts sizes the bars from the spacing of the data again, as in 6.1.0.
- The sub-daily behaviour from #44628 is unchanged, and its tests still pass.
- The docstring now says that only sub-daily grains get a grain-derived cap.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No screenshots from me. The issue has 6.1.0 vs 7.0.0rc2 screenshots of the same chart. The numbers come from the new tests, with an 800px chart and data from 2019-01-01 to 2024-01-01:
| Case | `barMaxWidth` before | after |
|---|---|---|
| Bar chart, Day grain, yearly data | 0.42px | 100 (default cap) |
| Bar chart, Day grain, monthly data | 2.27px | 100 |
| Bar chart, Month grain, yearly data | 12.90px | 100 |
| Mixed chart (bar series), Day grain, yearly data | 0.42px | 100 |
### TESTING INSTRUCTIONS
New tests:
- `plugins/plugin-chart-echarts/test/Timeseries/Bar/sparseSubDailyBarGeometry.test.ts`: a Day grain on yearly and on monthly data, and a Month grain on yearly data, keep the default bar width cap.
- `plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts`: the same for a mixed chart with a bar series, with a Day grain on yearly data.
All 4 fail on `master` (they receive the hairline widths above) and pass with this change.
Run locally with Node 24.16 / npm 11:
- `npx jest plugins/plugin-chart-echarts`: 101 suites, 1391 tests passed. The existing `PT1H` tests for #44628 still pass.
- `oxlint` and `oxfmt --check` on the changed files: clean.
- `node scripts/check-type.js package=superset-frontend <changed files>` (after `tsc --build` of the plugin project): no errors.
To check it manually, follow the steps in #45474: a Bar Chart on a dataset with one row per year, x-axis on the temporal column, Time Grain left at Day. The bars should have normal width, as in 6.1.0.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #45474
- [ ] Required feature flags:
- [x] 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
*Generative AI disclosure, following the [ASF guidelines](https://www.apache.org/legal/generative-tooling.html): I used Claude Code (Claude Opus 5.5) to help investigate this issue and draft the change, the tests and this description. I reviewed the result, checked that the new tests fail on `master`, and ran the checks above locally.*
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6511474721
@sfirke are we ok to merge? This one is going into 7.0.0rc3, and the screens have been added to the 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(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6291733396
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45519?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.15%. Comparing base ([`0a17a75`](https://app.codecov.io/gh/apache/superset/commit/0a17a75f9a87fd689b59010d506e122b156c35bf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aab7f5b`](https://app.codecov.io/gh/apache/superset/commit/aab7f5ba4a9816a90627dcb4d6c79585963657ae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45519 +/- ##
=======================================
Coverage 65.15% 65.15%
=======================================
Files 3053 3053
Lines 200640 200640
Branches 46820 46820
=======================================
Hits 130719 130719
Misses 66981 66981
Partials 2940 2940
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45519/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/45519/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.36% <100.00%> (ø)` | |
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/45519?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
Re: [PR] fix(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "xiaobai0529 (via GitHub)" <gi...@apache.org>.
xiaobai0529 commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6312881665
## Before / after screenshots from local Storybook testing
I independently rendered the PR's target case in Storybook from both revisions, using the same fixture, viewport, and chart dimensions.
**Setup**
- Before: `d5bccecf58126db88a596164daf6fdda9aa0d341`
- After: `aab7f5ba4a9816a90627dcb4d6c79585963657ae`
- Viewport: 960x900
- Chart width: 900px, height: 260px
- Bar chart, x-axis: temporal column, Time Grain: `P1D`
- Top chart: yearly rows (2019-2024); bottom chart: monthly rows (24 months)
**Before**
**After**
The before revision draws the yearly and monthly bars as hairlines because the Day-grain cap is far narrower than the spacing between rows. The after revision keeps the default 100px cap for Day and coarser grains, so the bars render at a normal width.
This was Storybook-level QA, not a full Superset web-server deployment. The fixture exercised `EchartsTimeseriesChartPlugin` and `TimeseriesTransformProps`, and both screenshots were captured after confirming that two chart canvases had rendered. I did not modify the PR branch.
The screenshot files are stored on a separate evidence branch of my public fork, not in the Superset repository.
--
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(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6521288412
I will try to deploy this today to kick the tires on my end.
--
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(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6289197046
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge.
- The grain-derived bar-width cap now applies only to sub-daily grains, avoiding narrow caps for daily or coarser grains on more widely spaced data.
- Tests cover daily or monthly grains on yearly and monthly data, plus mixed-timeseries yearly data.
**Assessed commit:** `aab7f5ba4a98`
--
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(plugin-chart-echarts): limit the grain-based bar width cap to sub-daily grains [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #45519:
URL: https://github.com/apache/superset/pull/45519#issuecomment-6527536531
@sadpandajoe and @villebro, please see the two options I laid out in my comment in the issue: https://github.com/apache/superset/issues/45474#issuecomment-6355816453 I am not sure that merging this PR is the right fix, it might be better to revert only the bar-width portion of https://github.com/apache/superset/pull/44628
--
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