fix column summary chart warnings and edge cases for null cols#10317
fix column summary chart warnings and edge cases for null cols#10317Light2Dark wants to merge 3 commits into
Conversation
…umns (marimo-team#10303) Column-summary histograms emitted Vega console warnings ("Dropping ... aggregate max", "Infinite extent") and had hover flicker on short bars. Root causes: - Tooltip/hover layers used `y: { aggregate: "max" }`, which Vega drops (no field) or shrinks to the bar height (with field), causing flicker. Replaced with full-height pixel hit targets (`y`/`y2`). - All-NaN / all-null columns produced empty quantitative histograms and fed NaN axis tick values into Vega. Now filtered via `isValidBinValue`, with a dedicated null-bar spec and NaN-safe axis ticks. - `getVegaSpec` mutated the stored bin values on every call by pushing a null bin; bins now derive from a fresh array. Also refactors the repeated null-bar/tooltip construction into a helper and expands the null/NaN column-summary smoke tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Architecture diagram
sequenceDiagram
participant UI as Table UI
participant Model as ColumnChartSpecModel
participant Bins as BinValues Store
participant Utils as isValidBinValue / calculateBinStep
participant SpecBuilder as buildNullBarSpec / FullHeightTooltip
participant Vega as Vega Rendering Engine
Note over UI,Vega: Column Summary Chart Rendering Flow
UI->>Model: getHeaderSummary(column)
Model->>Bins: get column bin values
Bins-->>Model: binValues (array)
Model->>Bins: get column stats (nulls count)
Bins-->>Model: nullCount
Model->>Utils: isValidBinValue(binValue)
Utils-->>Model: true/false
alt All bins invalid (all-NaN/all-null)
Model->>Model: Filter valid bins → empty array
alt nullCount > 0
Model->>SpecBuilder: buildNullBarSpec(width=CONCAT_CHART_WIDTH)
SpecBuilder-->>Model: null-only chart spec (vacant bar + invisible tooltip)
Model->>Vega: Render null bar only
Note over SpecBuilder: Uses y: { value: 0 }, y2: { value: 30 }
Note over SpecBuilder: No aggregate: max encoding
else No nulls
Model-->>UI: null spec (no chart)
end
else Valid bins exist
Model->>Model: Create fresh bins array (no mutation)
opt nullCount > 0
Model->>Model: Append synthetic null bin to fresh array
end
alt Numeric column
Model->>SpecBuilder: buildNullBarSpec(width=CONCAT_NULL_BAR_WIDTH, filterNullOnly)
SpecBuilder-->>Model: null bar spec
Model->>Model: Build histogram with transform filter (exclude null bins)
Model->>Model: Calculate axisTickValues (filter NaN)
Note over Model: Uses FULL_HEIGHT_TOOLTIP_Y instead of aggregate: max
Model->>Vega: Render hconcat(nullBar, histogram)
else Temporal column
Model->>Utils: getPartialTimeTooltip(validBins)
Model->>Model: Build temporal chart spec
alt Single non-null bin + no nulls
Model->>Vega: Full-width bar (no hconcat)
else Multiple bins
Model->>Vega: Layered chart with null bar + temporal bars
end
end
end
Note over Model,Vega: Key Design Decisions
Note over Model: Always creates fresh bins array - no stored state mutation
Note over SpecBuilder: Tooltip layers use pixel y/y2 values
Note over SpecBuilder: No aggregate: max encoding prevents hover flicker
Note over Utils: isValidBinValue rejects null, NaN, and string/Date NaN
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Pull request overview
Fixes Vega console warnings and hover flicker in data-table column-summary charts for all-null / all-NaN columns by making hover hit targets full-height, filtering invalid bin values, and preventing bin-state mutation across renders.
Changes:
- Replace tooltip/hover layers that used
aggregate: "max"with full-height pixel hit targets (y/y2withvalue) in both current and legacy specs. - Filter out invalid histogram bins (null/NaN) via shared
isValidBinValue, add NaN-safe axis tick values, and avoid mutating stored bin arrays when injecting a synthetic null bin. - Refactor null-bar construction into a helper and expand unit tests + smoke tests for null/NaN column scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| marimo/_smoke_tests/tables/column-header-chart.py | Expands visual smoke coverage for null/NaN numeric + temporal columns and threshold behavior. |
| marimo/_smoke_tests/pandas_smoke_tests/pandas_nan.py | Adds smoke scenarios for all-NaN and mixed NaN columns above/below chart threshold. |
| frontend/src/components/data-table/types.ts | Exports BinValue type so it can be referenced in shared bin-validation helpers. |
| frontend/src/components/data-table/column-summary/utils.ts | Adds isValidBinValue and uses it to avoid null/NaN-derived temporal tooltip/bin-step issues. |
| frontend/src/components/data-table/column-summary/legacy-chart-spec.ts | Updates legacy tooltip hit-target encodings to avoid Vega “Dropping aggregate max” warnings and hover flicker. |
| frontend/src/components/data-table/column-summary/chart-spec-model.tsx | Centralizes null-bar spec creation, filters invalid bins, avoids bin-array mutation, and makes tooltip hit areas full-height. |
| frontend/src/components/data-table/tests/chart-spec-model.test.ts | Adds/updates tests to cover all-null numeric/temporal, NaN-bin filtering, and non-mutation behavior. |
| frontend/src/components/data-table/tests/snapshots/chart-spec-model.test.ts.snap | Updates snapshots for new tooltip-layer encodings and new null-only chart specs. |
Comments suppressed due to low confidence (1)
frontend/src/components/data-table/column-summary/legacy-chart-spec.ts:158
y2: { value: 100 }is hard-coded to match the legacy chart height, but the actual height is controlled bybase.height. Derivingy2frombase.heightavoids the tooltip hit area drifting if the legacy chart height changes.
},
y: { value: 0 },
y2: { value: 100 },
tooltip: [
- Filter bin/axis-tick values with `Number.isFinite` instead of only rejecting `NaN`, so `±Infinity` from inf-containing columns can't leak into quantitative/temporal encodings (matches `isValidBinValue`'s "finite" docstring). - Replace the duplicated `100` legacy chart-height literal with a shared `DEFAULT_CHART_HEIGHT` constant used by both `createBase` and the legacy spec tooltip hit targets, so the hover area can't drift from the plot height. Co-authored-by: Cursor <cursoragent@cursor.com>
`getPartialTimeTooltip` bailed on `if (!value)`, which treats a valid numeric `0` (Unix epoch) bin start as missing. Since `isValidBinValue` already guarantees the found bin start is non-null and finite, switch to a nullish check so legitimate falsy values keep their tooltip. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
frontend/src/components/data-table/column-summary/utils.ts:12
- The JSDoc says this guard rejects “nulls and NaN”, but the implementation also rejects ±Infinity via
Number.isFinite. Update the comment so it matches the actual behavior (and keeps the “finite” wording consistent).
/**
* True when a bin has finite, non-null start/end suitable for quantitative /
* temporal encodings. Rejects nulls and NaN (all-NaN hist garbage).
*/
marimo/_smoke_tests/tables/column-header-chart.py:262
requests.get(...)without a timeout can hang indefinitely during manual smoke testing, and failures currently surface later as a zip parsing error. Consider adding a timeout andraise_for_status()so failures are fast and explicit.
response = requests.get(train_parquet_link)
zip_data = io.BytesIO(response.content)
Summary
Fixes #10303, the Vega console warnings and hover flicker in table column-summary charts reported in, and cleans up several related edge cases for null/NaN columns.
Root causes addressed:
Dropping ... aggregate maxwarning. The invisible tooltip/hover layers encodedy: { aggregate: "max" }. Without a field Vega drops the encoding (console warning); with a field the hit area shrinks to the (often tiny) bar height, causing flicker on short integer bars. Replaced with full-height pixel hit targets (y: { value: 0 },y2: { value: <height> }) in both the current and legacy numeric/temporal specs.Infinite extentwarnings on all-NaN / all-null columns. All-NaN bins produced empty quantitative histograms and fedNaNaxis tick values into Vega. Bins are now filtered through a sharedisValidBinValueguard (rejectsnullandNaN), all-null columns render a dedicated null bar only, and axis tick values areNaN-safe.getVegaSpecmutated the stored bin values on every call bypush-ing a synthetic null bin, so bins grew across renders. Chart data now derives from a fresh array and never mutates stored state.Also refactors the duplicated null-bar + full-height-tooltip construction into a single
buildNullBarSpechelper, and expands the null/NaN column-summary smoke tests (pandas_nan.py,column-header-chart.py).Test plan
pnpm test src/components/data-table/__tests__/chart-spec-model.test.ts(45 passing, incl. new all-null numeric/temporal, NaN-bin no-mutation, and empty-bins cases)make fe-check(typecheck + lint clean)make py-check(format + mypy clean)marimo/_smoke_tests/tables/column-header-chart.py, confirm no VegaInfinite extent/Dropping ... aggregate maxwarnings in the browser console and no hover flicker on short barsMade with Cursor