From c191824b162756a278312543c993a60808530128 Mon Sep 17 00:00:00 2001 From: debabsah Date: Sun, 4 Oct 2026 23:57:23 -0700 Subject: [PATCH 01/26] fix: advise and smoke ask one height question of tables and pivots The airline showcase build found three height disagreements on 6.1.0. The grid model was already shared; the questions asked of it were not. - size.table-window passed a table showing half its row_limit (table_visible_ratio 0.5) while smoke warns when any returned row hides, so a 25-row table at 13 units passed advise and failed smoke. The ratio is 1 for every audience and the target is every row, the number smoke gives once the data fills row_limit. - size.pivot-window read row_limit as the pivot's rows ("~0 of 1000 rows", raise to 331). row_limit caps records (rows x columns); a pivot draws a row per distinct row key. Offline the rule now checks only what the spec fixes (frame, header rows, totals row, first row) and reports without a fix; under --profile size.grid-fit counts the row dimensions and owns the chart, and table-window leaves probed tables to it too. - Smoke's pivot estimate was already right after the calibration (9 units for 5 rows and a totals row); the old "5.7 header units" bundled the totals row and scrollbar allowance. Messages now name each part. spec.grid_fit is the one estimate every consumer calls. pivot_axes, pivot_frame, pivot_rows and pivot_rows_from_counts follow 6.1.0's PivotTableChart.tsx and react-pivottable: transpose, metrics laid out as rows, row_subtotals and pivots without row dimensions are counted, where smoke counted `rows` for all of them. A table's show_totals row now takes a body row (checks and the page-length fill), and smoke queries a table or pivot with the compiler's row_limit default rather than 1,000. Design brain 8. tests/test_grid_rows.py checks that table-window and smoke, and grid-fit and smoke, warn together and name the same height. --- CHANGELOG.md | 11 + chartwright/design/brief.py | 4 +- chartwright/design/defaults.py | 6 +- chartwright/design/guidelines/chart-choice.md | 10 +- chartwright/design/model.py | 6 +- chartwright/design/presets.py | 17 +- chartwright/design/rules.py | 199 +++++++++----- chartwright/smoke.py | 47 ++-- chartwright/spec.py | 131 +++++++-- docs/DESIGN-BRAIN.md | 83 +++++- docs/VERIFICATION.md | 2 +- tests/test_design_arch.py | 4 +- tests/test_design_data.py | 3 +- tests/test_design_defaults.py | 9 +- tests/test_design_rules_v2.py | 6 +- tests/test_grid_rows.py | 251 ++++++++++++++++++ tests/test_standards.py | 4 +- tests/test_standards_content.py | 2 +- 18 files changed, 658 insertions(+), 137 deletions(-) create mode 100644 tests/test_grid_rows.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 193e8a9..b00cab0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,19 @@ while the major version is 0, minor bumps may include breaking changes and say s ## Unreleased +### Upgrading + +- The design brain is version 8. `advise` now flags a table whose height can't show every row its `row_limit` allows, as the smoke check after `apply` does when the data fills it; before, it passed a table that showed half. A `--design strict` or `advise --strict` gate can newly fail on such a table. To keep the old threshold for tables that rarely reach their `row_limit`, set `table_visible_ratio: 0.5` in `design.yaml` or a standards file. + ### Fixed +- `advise` and the smoke check after `apply` now ask the same height question of tables and pivots, so they no longer disagree: + - a table: advise names the height smoke names once the data fills `row_limit` (a 25-row table at 13 units now gets "raise height to ~21" from both); + - a pivot: `size.pivot-window` no longer reads `row_limit` as the pivot's rows. `row_limit` caps the query's records, which are rows times columns, so a 5-row cause by month pivot was told it showed "~0 of 1000 rows" and to grow to 331 units. Offline, the rule now checks only what the spec fixes (its header rows, totals row and first row) and says so; `advise --profile` counts each row dimension's values and gives the height smoke gives; + - messages name what takes the room: the card frame, each header row, the totals row and room for a horizontal scrollbar. +- Height estimates count what a pivot draws in every layout: a transposed pivot is sized by its column dimension, metrics laid out as rows multiply its rows, `row_subtotals` adds its subtotal rows, and a pivot with no row dimension is its totals row. A table's `show_totals` row now takes a row of height too, in the checks and in the page size `advise --fix` fills. +- The smoke check queries a table or pivot without a `row_limit` for as many rows as the chart does (10,000 for a pivot, 1,000 for a table), so a large pivot's height check counts all its rows. + - `decompile` (and so `adopt`) names more chart settings it can't carry when they differ from Superset's defaults: big-number font sizes and a hidden trendline, a smooth or step line, horizontal bars on a time axis, a pie's radius and hidden labels, a funnel's labels, tooltip and percentage calculation, treemap labels, a heatmap's axis sort, legend, margins, label intervals and value bounds, a histogram's normalize setting, and any chart's currency format. Before, they were dropped without a note. - `standards apply` no longer adds a header or footer row that the body already holds at its edge (the last rows for a footer, the first for a header). A dashboard built in the UI has no header or footer, so `decompile` and `adopt` read its legal line as a body row, and the standard's footer was then added below it, showing it twice. `--claim` moves such a row into the header or footer and records it; `explain` and the apply summary name it. diff --git a/chartwright/design/brief.py b/chartwright/design/brief.py index e157c6c..8aca8cb 100644 --- a/chartwright/design/brief.py +++ b/chartwright/design/brief.py @@ -44,7 +44,9 @@ def render_brief(audience: str, overlay: Overlay | None = None) -> str: f"- Vertical bars: <= {p.vbar_max_categories} categories, then flip horizontal; an ordered axis " f"(hours, ranks) stays vertical with category_sort and x_label_every. Pies: <= {p.pie_max_slices} slices.", f"- Timeseries: <= {p.series_max} grouped series (series_limit keeps the top N). Tables: height should " - f"show >= {p.table_visible_ratio:.0%} of row_limit (~0.8 units/row), or one page with page_length.", + f"show {'every row' if p.table_visible_ratio >= 1 else f'>= {p.table_visible_ratio:.0%}'} of " + f"row_limit (~0.75 units/row + 2.5), or one page with page_length. Pivots: a row per distinct " + f"row-dimension value (~0.65 units/row + ~4 to 6 for headers and totals), never per row_limit.", "- Charts sharing a row share a height; Superset sizes the row to its tallest child.", ] if p.recommended_heights: diff --git a/chartwright/design/defaults.py b/chartwright/design/defaults.py index dc86af2..496a55d 100644 --- a/chartwright/design/defaults.py +++ b/chartwright/design/defaults.py @@ -300,10 +300,12 @@ def _page_length(ctx: RuleContext, c): # The one grid model size.table-window reads (spec.py): a page fills the whole rows # that fit beside the page-size bar and the pager, so a fill can never make that # rule ask for more height. - fits = math.floor(grid_rows_visible(h, table_header_units(controls=c.search_box))) + fits = math.floor(grid_rows_visible(h, table_header_units(controls=c.search_box, + totals=c.show_totals))) if c.row_limit <= fits: return None, f"all {c.row_limit} rows fit at height {h:g}" - page = math.floor(grid_rows_visible(h, table_header_units(controls=True, pager=True))) + page = math.floor(grid_rows_visible(h, table_header_units(controls=True, pager=True, + totals=c.show_totals))) if page < ctx.params.page_min_rows: return None, f"height {h:g} fits {page} rows beside a pager, too few to page; raise the height" return page, (f"row_limit {c.row_limit} at height {h:g}: {fits} rows fit on one page, " diff --git a/chartwright/design/guidelines/chart-choice.md b/chartwright/design/guidelines/chart-choice.md index 92cb934..0d05b80 100644 --- a/chartwright/design/guidelines/chart-choice.md +++ b/chartwright/design/guidelines/chart-choice.md @@ -16,10 +16,12 @@ so a truncated pie lies about share. Prefer a horizontal bar for top-N; a truncated pie's title must say "top N". - Table and pivot height is a DATA-DEPENDENT property, not a one-time layout - choice: size from expected rows (~0.75 units per row + 3 for title/header, - +1 for pivot column headers) and re-check whenever a row dimension is - expected to gain members. Rows past the fold hide behind an inner - scrollbar; smoke warns at apply time when they do. + choice: size from expected rows (a table ~0.75 units per row + 2.5, more + with a search box, pager or totals row; a pivot ~0.65 per distinct + row-dimension value + 4 to 6 for its header rows and totals row, never from + its row_limit, which counts records: rows x columns) and re-check whenever a + row dimension is expected to gain members. Rows past the fold hide behind + an inner scrollbar; smoke warns at apply time when they do. - Sort order is per family: bars sort by their first metric (right for rankings, wrong for ordinals like weekday/month) unless `category_sort` orders them by label; heatmap and pivot categories sort alphabetically diff --git a/chartwright/design/model.py b/chartwright/design/model.py index ad50749..51a0ba5 100644 --- a/chartwright/design/model.py +++ b/chartwright/design/model.py @@ -26,7 +26,11 @@ # "7" = fills keep a null record when the author edits one (an edit later deleted stays # deleted), and default.stale-record: a fixable finding for a renamed or removed chart's # design.filled entry, which now validates. Both change what `--fix` writes. -DESIGN_BRAIN_VERSION = "7" +# "8" = one height question for tables and pivots (sec.15.42): table_visible_ratio 1 and +# every-row targets in size.table-window, size.pivot-window reading pivot rows instead of +# row_limit, size.grid-fit counting every pivot layout. No new rule, but a table can +# newly warn (and block a strict gate) and `--fix` writes different heights. +DESIGN_BRAIN_VERSION = "8" KPI_TYPES = {"big_number_total", "big_number_trend"} TIMESERIES_TYPES = {"timeseries_line", "timeseries_bar", "timeseries_area", "timeseries_scatter"} diff --git a/chartwright/design/presets.py b/chartwright/design/presets.py index 7151b31..d638bef 100644 --- a/chartwright/design/presets.py +++ b/chartwright/design/presets.py @@ -25,11 +25,12 @@ class Params: kpi_row_max: int kpi_height: int min_axis_height: int - # Min visible rows / row_limit. 0.5 everywhere: below half, the MAJORITY of - # the rows you deliberately asked for sit behind the grid's inner - # scrollbar -- the exact defect size.grid-fit and apply-time smoke exist to - # catch, so the offline rule must not bless it. Still a knob (deployments - # override per audience in design.yaml), just no longer a lenient default. + # Min visible rows / row_limit before size.table-window warns on an unpaged table. + # 1 everywhere: smoke warns after apply when any row the data fills row_limit + # with hides behind the grid's inner scrollbar, so at 0.5 advise passed a + # 25-row table at 13 units that smoke then said needed 21. Still a knob + # (design.yaml, per audience) for tables that rarely reach their row_limit; the + # rule's target height is every row whatever it is set to. table_visible_ratio: float vbar_max_categories: int pie_max_slices: int @@ -59,15 +60,15 @@ class Params: AUDIENCES: dict[str, Params] = { "executive": Params("executive", fold_units=22, max_row_charts=3, kpi_row_min=2, kpi_row_max=5, kpi_height=5, min_axis_height=8, - table_visible_ratio=0.5, vbar_max_categories=6, + table_visible_ratio=1.0, vbar_max_categories=6, pie_max_slices=5, series_max=5, max_filter_selects=5), "analytical": Params("analytical", fold_units=66, max_row_charts=4, kpi_row_min=2, kpi_row_max=6, kpi_height=4, min_axis_height=6, - table_visible_ratio=0.5, vbar_max_categories=8, + table_visible_ratio=1.0, vbar_max_categories=8, pie_max_slices=7, series_max=10, max_filter_selects=6), "operational": Params("operational", fold_units=22, max_row_charts=4, kpi_row_min=2, kpi_row_max=8, kpi_height=3, min_axis_height=5, - table_visible_ratio=0.5, vbar_max_categories=8, + table_visible_ratio=1.0, vbar_max_categories=8, pie_max_slices=7, series_max=8, max_filter_selects=7), } AUDIENCE_NAMES = tuple(AUDIENCES) diff --git a/chartwright/design/rules.py b/chartwright/design/rules.py index a9618a4..4244d79 100644 --- a/chartwright/design/rules.py +++ b/chartwright/design/rules.py @@ -22,11 +22,16 @@ DEFAULT_TIME_GRAIN, SUPERSET_COLOR_SCHEMES, HeaderBlock, + PIVOT_METRIC, _ColorSchemeMixin, - grid_header, + grid_fit, grid_rows_visible, grid_units_for_rows, + pivot_axes, + pivot_frame, + pivot_rows_from_counts, row_items, + table_page, without_superset_defaults, ) from .model import AXIS_TYPES, KPI_TYPES, TIMESERIES_TYPES, Finding, RuleContext, rule @@ -142,12 +147,64 @@ def heatmap_geometry(ctx: RuleContext): ) -def _table_page(c) -> int | None: - """Rows on one page of a paged table (page_length > 0), never more than row_limit; - None when every row is on one page.""" - if not getattr(c, "page_length", None): +_PROBE_CAP = 60 # distinct values a grid probe counts before it saturates + + +def _row_dims(c) -> list[str]: + """The dimensions a table's or pivot's body rows are keyed on: a pivot's row + attributes less the metric names (spec.pivot_axes), an aggregate table's groupby.""" + if c.type == "pivot_table": + return [a for a in pivot_axes(c)[0] if a != PIVOT_METRIC] + return list(c.groupby or []) if not c.columns else [] + + +def _counted(c, dims: list[str]) -> str: + """What a grid's row count counts, for a finding: the dimensions, and the metrics + when a pivot lays them out as rows.""" + what = " x ".join(repr(d) for d in dims) + if c.type == "pivot_table" and len(c.metrics) > 1 and PIVOT_METRIC in pivot_axes(c)[0]: + what = f"{what} x {len(c.metrics)} metrics" if what else f"{len(c.metrics)} metrics" + return what + + +def _probed_rows(ctx: RuleContext, c) -> tuple[int, int, bool] | None: + """(leaf rows, subtotal rows, exact) a table or pivot draws, from one bounded + distinct count per row dimension (advise --profile; the probes are cached). None + offline, when a probe fails, or for a chart a per-column count can't size (a raw + table, a grid with no row dimension). Exact with one dimension that didn't saturate + the probe; otherwise a lower bound. size.grid-fit reports it; size.table-window and + size.pivot-window leave a chart it sizes to that rule, so one advise run never + gives a chart two height targets.""" + dims = _row_dims(c) if c.type in ("table", "pivot_table") else [] + if ctx.prober is None or not dims or (ds := ctx.dataset_for(c)) is None: return None - return min(c.page_length, c.row_limit) if c.row_limit else c.page_length + # row_limit caps the records, so the distinct row keys too (a pivot's records are + # rows x columns, never fewer than its rows). + cap = c.row_limit or DEFAULT_ROW_LIMIT[c.type] + probe_cap = min(cap, _PROBE_CAP) + counts, saturated = {}, False + for d in dims: + n = ctx.prober.count_up_to(ds, d, probe_cap) + if n is None: + return None + saturated |= n > probe_cap and probe_cap < cap + counts[d] = min(n, cap) + if c.type == "pivot_table": + leaf, subtotals, exact = pivot_rows_from_counts(c, counts) + else: + leaf, subtotals, exact = min(max(counts.values()), cap), 0, len(dims) == 1 + return leaf, subtotals, exact and not saturated + + +def _pivot_overhead(c) -> str: + """What a pivot draws besides its body rows, in words (spec.pivot_frame).""" + header_rows, hscroll, totals = pivot_frame(c) + parts = ["the card frame", f"{header_rows} header row" + ("s" if header_rows != 1 else "")] + if totals: + parts.append("its totals row") + if hscroll: + parts.append("room for a horizontal scrollbar") + return ", ".join(parts[:-1]) + " and " + parts[-1] def _brain_page(ctx: RuleContext, c) -> bool: @@ -160,37 +217,42 @@ def _brain_page(ctx: RuleContext, c) -> bool: return ctx.brain_owns(c, "page_length") -@rule("size.table-window", "warn", "a table's height should show a meaningful share of its row_limit", - fixable=True) +@rule("size.table-window", "warn", "a table's height should show every row its row_limit allows, " + "or one whole page", fixable=True) def table_window(ctx: RuleContext): for c in ctx.spec.charts: if c.type != "table" or (c.row_limit is None and not c.page_length): continue + if _probed_rows(ctx, c) is not None: + continue # size.grid-fit sizes it by its real row count h = ctx.height(c.name) - # Same grid model as size.grid-fit, the fills and apply-time smoke (spec.py): - # offline this can only reason about row_limit (the ceiling), where grid-fit - # probes the real count, but all of them measure a table the same way. The - # header counts the search bar, the page-size bar and the pager the chart draws. - header, row = grid_header(c, c.row_limit) + # The question smoke asks after apply, of the most rows the table can render: + # row_limit (the ceiling, which a top-N or raw table reaches), or one page. Same + # grid_fit as size.grid-fit, the fills and smoke (spec.py); the header counts + # the search bar, the page-size bar, the pager and the Summary row it draws. + held, header, row = grid_fit(c, c.row_limit) visible = grid_rows_visible(h, header, row) # Whole rows, rounded down: rounding 6.67 up once told a 6-row page it showed # ~7 rows while asking for more height (the pager was what didn't fit). fits = f"table at {h:g} units fits {math.floor(visible)} full rows" - page = _table_page(c) + page = table_page(c) if page is not None: # A paged table shows one page at a time, plus its page controls: the whole # page should fit, but rows beyond it are a click away, not a scroll. want = page shown = f"{fits} beside its page controls, short of its {page}-row page" else: + # table_visible_ratio is 1 by default: the target is every row either way, + # so advise never passes a height smoke then flags when the data fills it. want = ctx.params.table_visible_ratio * c.row_limit - shown = f"{fits} of its {c.row_limit}" + shown = (f"{fits} of its {c.row_limit}; the rest hide behind an inner scrollbar " + f"once the data fills row_limit") if visible < want: brain = page is not None and _brain_page(ctx, c) - target = grid_units_for_rows(want, header, row) + target = grid_units_for_rows(held, header, row) yield Finding( "size.table-window", "warn", c.name, ctx.where(c.name), - f"{shown} (a scroll dungeon); " + f"{shown}; " + ("the page is a design default: advise --fix refits it to the panel" if brain else f"raise height to ~{math.ceil(target)} or " @@ -219,72 +281,89 @@ def hbar_window(ctx: RuleContext): ) -@rule("size.pivot-window", "warn", "a pivot's height should show a meaningful share of its row_limit", - fixable=True, since="2") +@rule("size.pivot-window", "warn", "a pivot's height must hold its header rows, totals row and the " + "body rows its spec fixes (advise --profile counts the rest)", fixable=True, since="2") def pivot_window(ctx: RuleContext): + """A pivot draws a body row per distinct row key, never per record: row_limit caps + the records (rows x columns), so it says nothing about the rows on screen. Offline + the spec fixes only a floor: a row per metric laid out as rows, at least one value + per row dimension, and no body row at all without row attributes (spec.pivot_rows). + The count beyond it is size.grid-fit's, from probes, and smoke's, from the data.""" for c in ctx.spec.charts: - if c.type != "pivot_table" or c.row_limit is None: - continue + if c.type != "pivot_table" or _probed_rows(ctx, c) is not None: + continue # a probed pivot is size.grid-fit's: it knows the real row count + dims = _row_dims(c) + leaf, subtotals, _ = pivot_rows_from_counts(c, dict.fromkeys(dims, 1)) + held, header, row = grid_fit(c, leaf + subtotals) + needed = grid_units_for_rows(held, header, row) h = ctx.height(c.name) - # Header rows per column dimension, the horizontal-scrollbar allowance and the - # pinned totals row: the shared grid model (spec.py), the one smoke reads. - header, row = grid_header(c) - visible = grid_rows_visible(h, header, row) - want = ctx.params.table_visible_ratio * c.row_limit - if visible < want: - target = grid_units_for_rows(want, header, row) + if needed <= h: + continue + frame = f"{_pivot_overhead(c)} ({round(header, 2):g} units)" + if not dims: + # Exact: without a row dimension the spec fixes every row the pivot draws. yield Finding( "size.pivot-window", "warn", c.name, ctx.where(c.name), - f"pivot shows ~{math.floor(visible)} of {c.row_limit} rows at {h:g} units " - f"({header:g} header units); raise height to " - f"~{math.ceil(target)} or lower row_limit", - fix=ctx.fix_height(c, math.ceil(target)) if target <= 20 else None, + f"pivot at {h:g} units: " + (f"its {held} rows beside " if held else "") + + f"{frame} need ~{math.ceil(needed)} units; raise height to ~{math.ceil(needed)}", + fix=ctx.fix_height(c, math.ceil(needed)) if needed <= 20 else None, height_driven=True, ) + continue + # A floor only, so no fix: one that still hides rows would only move the warning + # to smoke. The count that sizes the pivot comes from --profile or the data. + first = "the first row" if held == 1 else f"the first {held} rows" + per = " x ".join(repr(d) for d in dims) + " value" + ( + " and metric" if len(c.metrics) > 1 and PIVOT_METRIC in pivot_axes(c)[0] else "") + yield Finding( + "size.pivot-window", "warn", c.name, ctx.where(c.name), + f"pivot at {h:g} units holds {math.floor(grid_rows_visible(h, header, row))} " + f"full rows beside {frame}; with a row per {per} it needs " + f"~{math.ceil(needed)} units for {first} and {row:g} for each more: " + f"advise --profile counts them", + height_driven=True, + ) @rule("size.grid-fit", "warn", "table/pivot heights must fit their data-driven row counts (they grow after authoring)", fixable=True, data_aware=True, since="2") -def grid_fit(ctx: RuleContext): +def grid_fit_rule(ctx: RuleContext): """Pre-apply half of issue #1 (the apply-time half lives in smoke): probe the actual dimension cardinality and check the configured height fits. - Honest scope: single row-dimension pivots and single-groupby aggregate - tables -- multi-dim leaf counts aren't knowable from per-column probes.""" + Exact for one row dimension (a single-groupby aggregate table, a pivot with one + row dimension, its metrics laid out as rows or not); with more, per-column probes + give only a lower bound (the largest count), reported without a fix.""" if ctx.prober is None: return for c in ctx.spec.charts: - if c.type == "pivot_table" and len(c.rows) == 1: - dim = c.rows[0] - elif c.type == "table" and (c.groupby or []) and len(c.groupby) == 1 and not c.columns: - dim = c.groupby[0] - else: + probed = _probed_rows(ctx, c) + if probed is None: continue - ds = ctx.dataset_for(c) - if ds is None: - continue - cap = c.row_limit or 60 - n = ctx.prober.count_up_to(ds, dim, min(cap, 60)) - if n is None: - continue - n = min(n, cap) - # Shared grid model (chartwright/spec.py), same numbers smoke uses: the - # header counts the controls, pager, header rows and totals the chart draws. - header, row = grid_header(c, n) - page = _table_page(c) if c.type == "table" else None - if page is not None: - n = min(n, page) # one page (its pager is in the header), not every row - needed = math.ceil(grid_units_for_rows(n, header, row)) + leaf, subtotals, exact = probed + # Shared grid model (chartwright/spec.py), the grid_fit smoke calls: the header + # counts the controls, pager, header rows and totals the chart draws, and a + # paged table holds one page (its pager is in the header), not every row. + held, header, row = grid_fit(c, leaf + subtotals) + needed = math.ceil(grid_units_for_rows(held, header, row)) h = ctx.height(c.name) if needed <= h: continue # A brain-filled page follows the height: report, but leave the fix to the fill. - brain = page is not None and _brain_page(ctx, c) + brain = c.type == "table" and table_page(c) is not None and _brain_page(ctx, c) + totals = pivot_frame(c)[2] if c.type == "pivot_table" else c.show_totals + about = "~" if exact else "at least " + paged = held != leaf + subtotals # more rows than a page: the page is exact yield Finding( "size.grid-fit", "warn", c.name, ctx.where(c.name), - f"{dim!r} yields ~{n} rendered rows needing ~{needed} units; height {h:g} " - f"hides the tail behind an inner scrollbar -- and row counts grow with the " - f"data, so this only gets worse", - fix=ctx.fix_height(c, needed) if needed <= 20 and not brain else None, + f"{_counted(c, _row_dims(c))} yields {about}{leaf} rendered rows" + + (f", {subtotals} subtotal rows" if subtotals else "") + + (" and a totals row" if totals else "") + + (f"; its {held}-row page needs ~{needed} units" if paged + else f" needing {about}{needed} units") + + f"; height {h:g} hides the tail behind an inner scrollbar -- and row counts " + f"grow with the data, so this only gets worse", + fix=ctx.fix_height(c, needed) if (exact or paged) and needed <= 20 and not brain + else None, height_driven=True, ) diff --git a/chartwright/smoke.py b/chartwright/smoke.py index 2a9cce8..b28417a 100644 --- a/chartwright/smoke.py +++ b/chartwright/smoke.py @@ -19,7 +19,8 @@ from .client import SupersetClient from .compiler import _metric_payload, mixed_time_axis, time_binding from .resolver import Resolution -from .spec import DashboardSpec, grid_header, grid_units_for_rows +from .spec import (DEFAULT_ROW_LIMIT, DashboardSpec, grid_fit, grid_units_for_rows, pivot_frame, + pivot_rows) def _fit_warning(chart, spec: DashboardSpec, result: list) -> str | None: @@ -36,29 +37,32 @@ def _fit_warning(chart, spec: DashboardSpec, result: list) -> str | None: if not data: return None if chart.type == "pivot_table": - if not chart.rows: - return None # columns-only pivot: a single metric band, height-safe - leaf = len({tuple(r.get(d) for d in chart.rows) for r in data}) - header, row = grid_header(chart) - what = f"pivot renders ~{leaf} leaf rows" + (" and its totals row" if chart.column_totals else "") + # The rows the pivot draws, not the records the query returned (rows x columns). + leaf, subtotals = pivot_rows(chart, data) + rows = leaf + subtotals + what = ("pivot renders only its totals row" if not rows else # no row attribute + f"pivot renders ~{leaf} leaf rows" + + (f", {subtotals} subtotal rows" if subtotals else "") + + (" and its totals row" if pivot_frame(chart)[2] else "")) + remedy = "filter it to fewer rows" # row_limit counts records, not pivot rows else: - leaf = len(data) # already capped by the query's row_limit - header, row = grid_header(chart, leaf) - what = f"table renders ~{leaf} rows" - if chart.page_length and leaf > chart.page_length: - # A paged table renders one page and its page controls (size.table-window's model). - leaf = chart.page_length - what = f"table renders a {chart.page_length}-row page and its pager" + rows = len(data) # already capped by the query's row_limit + what = f"table renders ~{rows} rows" + (" and its totals row" if chart.show_totals else "") + remedy = "cap row_limit" + held, header, row = grid_fit(chart, rows) + if held != rows: + # A paged table renders one page and its page controls (size.table-window's model). + what = f"table renders a {held}-row page and its pager" height = spec.resolved_height(chart.name) - # Shared grid model (chartwright/spec.py): the design critic's - # size.grid-fit and size.table-window read the same numbers, so pre-apply - # advice and this post-apply warning can never contradict each other. - needed = grid_units_for_rows(leaf, header, row) + # Shared grid model (chartwright/spec.py): the design critic's size.grid-fit, + # size.pivot-window and size.table-window ask grid_fit the same question, so + # pre-apply advice and this post-apply warning can never contradict each other. + needed = grid_units_for_rows(held, header, row) if needed <= height: return None return (f"{what} (~{needed * 40:.0f}px) but height={height:g} ({height * 40:.0f}px): " f"rows will hide behind an inner scrollbar; raise height to " - f"~{math.ceil(needed)} units or cap row_limit") + f"~{math.ceil(needed)} units or {remedy}") @dataclass @@ -117,11 +121,16 @@ def _time_axis(column: str, grain: str | None) -> dict: def _query_for(chart, spec: DashboardSpec) -> dict: slug = spec.dashboard.slug + # A table or pivot asks for the rows its compiled chart asks for (the compiler's + # default when row_limit is unset), so the height fit counts every row it draws: a + # 1,000-record cap misses leaf rows of a pivot riding its 10,000 default. + grid = chart.type in ("table", "pivot_table") q: dict = { "filters": _filters_payload(chart), "extras": {"time_grain_sqla": getattr(chart, "time_grain", None) or "P1D"}, "time_range": "No filter", - "row_limit": getattr(chart, "row_limit", None) or 1000, + "row_limit": getattr(chart, "row_limit", None) or ( + DEFAULT_ROW_LIMIT[chart.type] if grid else 1000), "columns": [], "metrics": [], "orderby": [], diff --git a/chartwright/spec.py b/chartwright/spec.py index 9912ad3..445cd22 100644 --- a/chartwright/spec.py +++ b/chartwright/spec.py @@ -31,7 +31,9 @@ # How a rendered table or pivot spends spec height. ONE model, read by the design # critic (size.table-window, size.pivot-window, size.grid-fit), the page-length and # search-box fills, and apply-time smoke, so offline advice, data-aware advice, the -# fills and the apply warning can never disagree about the same chart. +# fills and the apply warning can never disagree about the same chart. Every consumer +# asks grid_fit() what a chart drawing N body rows needs; they differ only in where N +# comes from (the rows a query returned, a probed count, or the row_limit ceiling). # # Measured 2026-10-03 on rendered Superset 4.1.4, 5.0.0 and 6.1.0 (1600 px viewport, # default theme; docs/DESIGN-BRAIN.md, "Calibration"). Each constant is the largest @@ -42,14 +44,23 @@ GRID_CONTROLS_UNITS = 1.05 # the bar above the rows that search_box or any page_length # adds: 41.1 px / 39.1 px (6.1.0 draws 33.1 px for a page size alone) GRID_PAGER_UNITS = 1.45 # the pager under a table of more than one page: 57.9 px / 42.9 px -PIVOT_UNITS_PER_ROW = 0.65 # pivot body row: 25.8 px on average (19 rows: 490.1 px) +GRID_TOTALS_UNITS = 0.74 # show_totals' Summary row: the table foot, which DataTable keeps + # out of the scrolling body (6.1.0 plugin-chart-table DataTable/ + # hooks/useSticky.tsx:183,217); not measured, so one body row +PIVOT_UNITS_PER_ROW = 0.65 # pivot body row: 25.8 px on average (19 rows: 490.1 px), as 6.1.0's + # styles give it: 12 px text at line-height 1.4, 4 px padding above + # and below and a 1 px rule (react-pivottable/Styles.ts:27,33,113,115) PIVOT_FRAME_UNITS = 2.55 # pivot card title, margins and padding: 101 px -PIVOT_HEADER_ROW_UNITS = 0.66 # one pivot header row: 26.3 px; a pivot draws one per column - # dimension plus two (the metric row and the row-dimension names) +PIVOT_HEADER_ROW_UNITS = 0.66 # one pivot header row: 26.3 px (pivot_frame counts them) PIVOT_TOTALS_UNITS = 0.72 # the totals row column_totals pins over the bottom rows: 28.8 px + # (sticky at the bottom, Styles.ts:55-59) PIVOT_HSCROLL_UNITS = 0.45 # the horizontal scrollbar under a pivot whose column dimensions # outgrow the panel: 11 px in Chromium's classic scrollbars, and - # Windows draws 17 px; overlay scrollbars (macOS) take none + # Windows draws 17 px; overlay scrollbars (macOS) take none. + # 6.1.0 styles its own at 8 px (PivotTableChart.tsx:70-73) +# The metric names: one more attribute of a pivot's rows or columns, which Superset adds +# even for a single metric (6.1.0 plugin-chart-pivot-table PivotTableChart.tsx:98,382-386). +PIVOT_METRIC = "\x00metric" def grid_units_for_rows(rows: float, header_units: float = GRID_HEADER_UNITS, @@ -64,35 +75,119 @@ def grid_rows_visible(height: float, header_units: float = GRID_HEADER_UNITS, return max(0.0, (height - header_units) / row_units) -def table_header_units(*, controls: bool = False, pager: bool = False) -> float: +def table_header_units(*, controls: bool = False, pager: bool = False, + totals: bool = False) -> float: """Everything a table draws besides its body rows: card title, column header and padding, plus the bar above the rows (`controls`: a search box, or any - page_length, which draws a page-size picker even on one page) and the pager under - them (`pager`: more rows than one page holds).""" + page_length, which draws a page-size picker even on one page), the pager under + them (`pager`: more rows than one page holds) and the Summary row (`totals`: + show_totals).""" return (GRID_HEADER_UNITS + (GRID_CONTROLS_UNITS if controls else 0) - + (GRID_PAGER_UNITS if pager else 0)) + + (GRID_PAGER_UNITS if pager else 0) + (GRID_TOTALS_UNITS if totals else 0)) + + +def pivot_axes(chart) -> tuple[list[str], list[str]]: + """The (row, column) attributes a pivot draws, as 6.1.0's PivotTableChart.tsx:377-386 + builds them: transpose swaps the dimensions, and the metric names (PIVOT_METRIC) are + one more attribute, on the columns unless metrics_layout is rows; outermost, or + innermost with combine_metric.""" + rows, cols = ((list(chart.columns), list(chart.rows)) if chart.transpose + else (list(chart.rows), list(chart.columns))) + side = rows if chart.metrics_layout == "rows" else cols + side.insert(len(side) if chart.combine_metric else 0, PIVOT_METRIC) + return rows, cols + + +def pivot_frame(chart) -> tuple[int, bool, bool]: + """(header rows, horizontal-scrollbar room, totals row) of a pivot as configured. + 6.1.0's TableRenderers.tsx draws a header row per column attribute, and one naming + the row attributes when there are any (:1418-1423); the totals row with + column_totals, and always when there are no row attributes (:367, :1429). Column + dimensions, not metric names, are what can outgrow the panel.""" + rows, cols = pivot_axes(chart) + return (len(cols) + (1 if rows else 0), any(a != PIVOT_METRIC for a in cols), + chart.column_totals or not rows) + + +def pivot_header_units(header_rows: int, *, hscroll: bool = False, + totals: bool = False) -> float: + """Everything a pivot draws besides its body rows: card frame, its header rows, + room for a horizontal scrollbar (`hscroll`) and the pinned totals row (`totals`).""" + return (PIVOT_FRAME_UNITS + PIVOT_HEADER_ROW_UNITS * header_rows + + (PIVOT_HSCROLL_UNITS if hscroll else 0) + + (PIVOT_TOTALS_UNITS if totals else 0)) -def pivot_header_units(column_dims: int, *, totals: bool = False) -> float: - """Everything a pivot draws besides its body rows: card frame, a header row per - column dimension plus two, room for a horizontal scrollbar when column dimensions - can outgrow the panel, and the pinned totals row (`column_totals`).""" - return (PIVOT_FRAME_UNITS + PIVOT_HEADER_ROW_UNITS * (column_dims + 2) - + (PIVOT_HSCROLL_UNITS if column_dims else 0) - + (PIVOT_TOTALS_UNITS if totals else 0)) +def pivot_rows(chart, records: list[dict]) -> tuple[int, int]: + """(leaf rows, subtotal rows) a pivot draws for its query's records: a body row per + distinct row key, never per record (the query returns rows x columns records). The + metric names count as a row attribute when laid out as rows, and row_subtotals adds + a row per distinct key prefix (6.1.0 react-pivottable/utilities.ts:957-975). With no + row attribute there is no body row: the pivot is its totals row.""" + rows, _ = pivot_axes(chart) + if not rows: + return 0, 0 + labels = [metric_label(m) for m in chart.metrics] if PIVOT_METRIC in rows else [None] + keys = {tuple(label if a == PIVOT_METRIC else r.get(a) for a in rows) + for r in records for label in labels} + subtotals = (len({k[:i] for k in keys for i in range(1, len(rows))}) + if chart.row_subtotals else 0) + return len(keys), subtotals + + +def pivot_rows_from_counts(chart, counts: dict[str, int]) -> tuple[int, int, bool]: + """(leaf rows, subtotal rows, exact) a pivot draws, from the distinct values of each + row dimension counted one column at a time (`counts`). Exact with at most one row + dimension; with more, the combinations number at least the largest count, so the + leaf rows are a lower bound and subtotals go uncounted (exact False).""" + rows, _ = pivot_axes(chart) + if not rows: + return 0, 0, True + size = {a: len(chart.metrics) if a == PIVOT_METRIC else counts[a] for a in rows} + dims = [a for a in rows if a != PIVOT_METRIC] + if len(dims) > 1: + per = len(chart.metrics) if PIVOT_METRIC in rows else 1 + return max(size[a] for a in dims) * per, 0, False + subtotals = (sum(math.prod(size[a] for a in rows[:i]) for i in range(1, len(rows))) + if chart.row_subtotals else 0) + return math.prod(size.values()), subtotals, True + + +def table_page(chart) -> int | None: + """Rows on one page of a paged table (page_length > 0), never more than row_limit; + None when every row is on one page.""" + if not getattr(chart, "page_length", None): + return None + return min(chart.page_length, chart.row_limit) if chart.row_limit else chart.page_length def grid_header(chart, rows: int | None = None) -> tuple[float, float]: """(header units, units per body row) of a table or pivot chart as configured, drawing `rows` body rows (None: unknown, so a paged table counts its pager).""" if chart.type == "pivot_table": - return (pivot_header_units(len(chart.columns), totals=chart.column_totals), + header_rows, hscroll, totals = pivot_frame(chart) + return (pivot_header_units(header_rows, hscroll=hscroll, totals=totals), PIVOT_UNITS_PER_ROW) page = chart.page_length or 0 return (table_header_units(controls=bool(page) or chart.search_box, - pager=bool(page) and (rows is None or rows > page)), + pager=bool(page) and (rows is None or rows > page), + totals=chart.show_totals), GRID_UNITS_PER_ROW) + +def grid_fit(chart, rows: int | None) -> tuple[int | None, float, float]: + """(body rows the panel must hold, header units, units per body row) of a table or + pivot drawing `rows` body rows: the one estimate that smoke (the rows the query + returned), size.grid-fit (a probed count), size.pivot-window (what the spec alone + fixes) and size.table-window (the row_limit ceiling) each compare a height with. A + paged table holds one page beside its pager. None: unknown (a paged table still + holds one page).""" + header, row = grid_header(chart, rows) + page = table_page(chart) if chart.type == "table" else None + if page is not None and (rows is None or rows > page): + rows = page + return rows, header, row + ADHOC_AGGREGATES = ("SUM", "AVG", "COUNT", "COUNT_DISTINCT", "MIN", "MAX") _ADHOC_RE = re.compile(r"^(SUM|AVG|COUNT|COUNT_DISTINCT|MIN|MAX)\((.+?)\)(?:\s+AS\s+(.+))?$") # A custom-SQL metric: SQL() AS