sfirke commented on issue #45474:
URL: https://github.com/apache/superset/issues/45474#issuecomment-6355816453
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._:
@sadpandajoe, I'd like your input on how to fix #45474 before we merge
anything. The cause is the bar-width cap from #44628, and there are two
options: the narrow fix in #45519, or reverting just the bar-width half of
#44628. Either way, the coltypes/`is_dttm` fallback for the x-axis should stay.
It fixes a separate bug and isn't involved here.
### What the cap assumes
`getGrainBarMaxWidth` sizes each bar to one time-grain bucket, measured
across the full date span of the data. That assumes each row is one grain
bucket, and that the chart always shows the full date span. Two things break
those assumptions:
- **Rows spaced further apart than the grain.** This issue is the common
case: yearly rows with the default Day grain. Each bar is 1/1826 of the plot,
about 0.4px.
- **Zoom.** I rendered this to check. I used one week of hourly data with
every hour populated, an Hour grain, zoom on, and an 800px chart. I ran
master's `transformProps` output through ECharts 6.1.0's SVG renderer and read
the bar widths from ECharts' layout:
| | Full range | Zoomed to ~17 hours |
|---|---|---|
| master (cap = 4.55px) | 3.0px | **4.6px** |
| 6.1 (`barMaxWidth: 100`) | 3.0px | **30.5px** |
ECharts recalculates bar width for the zoomed range, but the cap is a
fixed pixel value computed for the full range. Zooming in only spreads the bars
apart; it never makes them wider. This happens with well-shaped, non-sparse
data.
Master, zoomed in:
<img width="800" height="400" alt="Image"
src="https://github.com/user-attachments/assets/99c7107c-fdab-49a2-96c7-f868540fc4c8";
/>
6.1 behavior, same zoom:
<img width="800" height="400" alt="Image"
src="https://github.com/user-attachments/assets/286c6ce5-3f2c-4808-9c81-cc313180133b";
/>
### Option A: merge #45519 (apply the cap only below Day)
**Pros**
- Small, easy to review, and has tests. Good fit for 7.0 at the RC stage.
- Fixes every case reported in #45474: Day or Month grain on monthly or
yearly data.
- Keeps the out-of-the-box fix for sparse hourly and minute charts that
#44628 was written for.
**Cons**
- Every zoomable bar chart with a sub-daily grain keeps the zoom problem
above. That includes charts on dense, well-shaped data, so it reaches beyond
the sparse case #44628 targeted.
- The spacing mismatch can still happen below Day:
- Daily rows viewed with an Hour grain: 30 days on an 800px chart gives
bars about 1px wide.
- Hourly readings with a 15-minute grain: bars are a quarter of their 6.1
width.
The easiest way to hit this is a dashboard Time Grain filter. It applies
to every chart in scope, so choosing Hour to look closely at one chart also
shrinks the bars on charts built on daily data.
- We keep about 100 lines of sizing code whose results depend on the grain
matching the data and the chart never being zoomed.
### Option B: revert the bar-width part of #44628
This means removing `getGrainBarMaxWidth`, its two call sites, and the
comment in `transformers.ts`.
**Pros**
- Restores the 6.1 bar sizing, which shipped for a long time without
complaints like #45474.
- Fixes the spacing mismatch for every grain, and fixes the zoom problem.
ECharts sizes bars from the smallest gap between x-values in the visible range,
so bars come out right whenever the data is on a regular grid.
- Users can already handle the sparse sub-daily case without new code:
Advanced Analytics → Resample, rule matching the grain (e.g. `1H`), with "Fill
the entire time range" (#42587, new in 7.0).
- Null imputation is enough. ECharts only needs the x positions, and
Superset keeps null rows, so nobody has to make up zeros for metrics where zero
would be wrong.
- "Fill the entire time range" covers the single-populated-bucket case,
which resampling couldn't handle before 7.0.
**Cons**
- Sparse hourly and minute bar charts go back to wide bars by default. Users
have to find the Resample controls, which are not easy to discover. The rule
dropdown also has no 15-minute option, though you can type `15min`. _Though I
think this is the proper way for a user to fix this, and it's good for them to
discover this approach._
- The resample rule is fixed in the chart, so it doesn't follow a dashboard
grain change. Switching a dashboard to Day while a chart resamples at `1H`
gives wrong results: 23 empty hourly slots between daily bars. #44628 does
follow the dashboard grain.
- It removes work that was reviewed and approved, and I assume a real user
report is behind it.
### Questions for you
1. What was the original case for #44628? Would resampling with "Fill the
entire time range" have solved it, or did it need to work automatically?
2. Given the zoom result, does keeping the cap for sub-daily grains still
seem worth it to you? Or do you see a sizing rule that works with zoom and
doesn't assume the data matches the grain?
--
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]
