From c02a6ab51bdbb481c0fb98551cd72c53c62839c9 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 1 Jul 2026 16:32:37 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20chart-type-aware=20row=20cap=20?= =?UTF-8?q?=E2=80=94=20chartRowCap(type)=20replaces=20flat=20CHART=5FROW?= =?UTF-8?q?=5FCAP=3D500?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each chart shape has a different readable ceiling (pie legibility tops out ~30 slices; bar/column are bar-width bound; line/area are point- density bound and can plot thousands before blurring), so a single flat cap either truncates too aggressively (pie) or too conservatively (line/area — a 1.4K-row result only plotted its first 500 points). CHART_ROW_CAPS / chartRowCap(type) in src/core/chart-data.js replace the constant; buildChartData() and the results.js truncation note both key off cfg.type, and switching chart type re-slices + updates the note via the existing rerender() path. Closes #109 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01784dKCpk5W7rdpUAwcQnix --- CHANGELOG.md | 6 ++++++ src/core/chart-data.js | 18 ++++++++++++++---- src/ui/results.js | 17 ++++++++++------- tests/unit/chart-data.test.js | 26 +++++++++++++++++++++----- tests/unit/results.test.js | 23 ++++++++++++++++++++++- 5 files changed, 73 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b15c14..cb8cb77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -157,6 +157,12 @@ auto-generated per-PR notes; this file is the curated, human-readable history. addendum); the schema slice is the documented imperative exception, converted with a *replaced* Set-valued `expanded` signal and reference-replaced column loads rather than in-place mutation. This **completes the migration**. (#88, #91) +- **Chart-type-aware row cap** (#109): the flat 500-row chart cap is now a + per-type lookup (`chartRowCap(type)` / `CHART_ROW_CAPS` in + `src/core/chart-data.js`) — Pie 30, Bar (horizontal) 500, Column 1000, Line/ + Area 5000 — matching each chart shape's actual readability ceiling instead + of one eyeballed number. Switching chart type re-slices to the new cap and + updates the truncation note in lockstep. ### Fixed - A newly created, still-empty database (e.g. `CREATE DATABASE`) never appeared diff --git a/src/core/chart-data.js b/src/core/chart-data.js index 62d93c3..a464fff 100644 --- a/src/core/chart-data.js +++ b/src/core/chart-data.js @@ -14,9 +14,19 @@ const TIME_RE = /^(Date|DateTime)/; // than being misclassified by a mere prefix and dropped from autoChart. const ORDINAL_RE = /^(year|quarter|month|week|day|dayofweek|dow|hour|minute)s?$/i; -// Plots past this get unreadable, so the chart shows the first N (the table -// stays full). Exported so the renderer can surface the truncation to the user. -export const CHART_ROW_CAP = 500; +// Plots past this get unreadable, so each chart type shows only its first N +// rows (the table stays full) — the readable ceiling differs by shape: pie +// legibility caps out around 20-30 slices regardless of monitor width; bar/ +// column are bound by minimum bar+gap width for legible category ticks; +// line/area are point-density bound, so a wide canvas can plot thousands of +// points before individual ones blur together. Exported so the renderer can +// surface the truncation to the user. +export const CHART_ROW_CAPS = { pie: 30, hbar: 500, bar: 1000, line: 5000, area: 5000 }; + +/** The row cap for a chart type, falling back to 500 (the old flat cap) for an unmapped type. */ +export function chartRowCap(type) { + return CHART_ROW_CAPS[type] ?? 500; +} /** Strip `Nullable(...)` / `LowCardinality(...)` wrappers down to the base type. */ export function chartStripType(type) { @@ -233,7 +243,7 @@ export function chartColors(read) { * - otherwise: one dataset per measure in `cfg.y`. */ export function buildChartData(columns, rows, cfg) { - const slice = rows.slice(0, CHART_ROW_CAP); + const slice = rows.slice(0, chartRowCap(cfg.type)); const num = (v) => (v == null || v === '' ? 0 : Number(v) || 0); const cats = []; // raw X keys, first-seen order const seen = new Set(); diff --git a/src/ui/results.js b/src/ui/results.js index 92fa5db..abdd4e8 100644 --- a/src/ui/results.js +++ b/src/ui/results.js @@ -8,7 +8,7 @@ import { loadingPlaceholder } from './placeholder.js'; import { formatRows, formatBytes, isNumericType } from '../core/format.js'; import { looksLikeHtml, prettyValue } from '../core/cell.js'; import { sortRows } from '../core/sort.js'; -import { autoChart, schemaKey, chartFieldOptions, chartColors, chartJsConfig, chartCfgValid, normalizeChartCfg, unzoomChartEvent, CHART_ROW_CAP } from '../core/chart-data.js'; +import { autoChart, schemaKey, chartFieldOptions, chartColors, chartJsConfig, chartCfgValid, normalizeChartCfg, unzoomChartEvent, chartRowCap } from '../core/chart-data.js'; import { EXPLAIN_VIEWS } from '../core/explain.js'; import { SELECT_ROW_CAP } from '../core/script-result.js'; import { RESULT_ROW_LIMIT_OPTIONS } from '../state.js'; @@ -971,19 +971,22 @@ export function renderChart(app, r, opts = {}) { rerender(); })); } - // The chart plots at most CHART_ROW_CAP points; say so when the result is - // bigger (the table still shows everything) — no silent truncation. - if (r.rows.length > CHART_ROW_CAP) { + // The chart plots at most cap points for the current type; say so when the + // result is bigger (the table still shows everything) — no silent + // truncation. Recomputed on every rerender (the Type select's onChange), + // so switching type re-slices and updates the note in lockstep. + const cap = chartRowCap(cfg.type); + if (r.rows.length > cap) { bar.appendChild(h('span', { class: 'chart-cap-note' }, - 'first ' + CHART_ROW_CAP + ' of ' + formatRows(r.rows.length) + ' rows')); + 'first ' + cap + ' of ' + formatRows(r.rows.length) + ' rows')); } const canvas = h('canvas', null); // via h() so it lands in the right document (detached-tab safe) // Plot in result (query) order — independent of the table's sort, which is a // global, cross-tab setting; applying it here would reorder the X axis (a - // time series would zig-zag) and change which rows the CHART_ROW_CAP keeps, + // time series would zig-zag) and change which rows the type's row cap keeps, // contradicting the "first N rows" note. It would also sort up to VIS_CAP - // rows just to discard all but the first CHART_ROW_CAP. + // rows just to discard all but the first `cap`. const chart = installChartZoomFix( new app.Chart(canvas, chartJsConfig(r.columns, r.rows, cfg, chartColors(app.cssVar))), canvas); diff --git a/tests/unit/chart-data.test.js b/tests/unit/chart-data.test.js index 5423f10..88f9e3e 100644 --- a/tests/unit/chart-data.test.js +++ b/tests/unit/chart-data.test.js @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { chartStripType, chartRole, autoChart, schemaKey, CHART_TYPES, chartFieldOptions, chartNumFmt, chartLabel, chartPalette, chartColors, buildChartData, chartJsConfig, - cloneChartCfg, chartCfgValid, normalizeChartCfg, unzoomChartEvent, + cloneChartCfg, chartCfgValid, normalizeChartCfg, unzoomChartEvent, chartRowCap, } from '../../src/core/chart-data.js'; describe('chartStripType', () => { @@ -198,6 +198,18 @@ describe('chartColors', () => { }); }); +describe('chartRowCap', () => { + it('returns the per-type cap, falling back to the bar/column default for unknown types', () => { + expect(chartRowCap('pie')).toBe(30); + expect(chartRowCap('hbar')).toBe(500); + expect(chartRowCap('bar')).toBe(1000); + expect(chartRowCap('line')).toBe(5000); + expect(chartRowCap('area')).toBe(5000); + expect(chartRowCap('bogus')).toBe(500); + expect(chartRowCap(undefined)).toBe(500); + }); +}); + describe('buildChartData', () => { const cols = [ { name: 'carrier', type: 'String' }, @@ -227,11 +239,15 @@ describe('buildChartData', () => { { label: 'W', data: [30, 20] }, // W has B6(30) and AA(20) ]); }); - it('caps at the row cap', () => { + it('caps at the row cap for the config type', () => { + const bigCols = [{ name: 'c', type: 'String' }, { name: 'n', type: 'UInt64' }]; const big = Array.from({ length: 600 }, (_, i) => ['c' + i, String(i)]); - const out = buildChartData([{ name: 'c', type: 'String' }, { name: 'n', type: 'UInt64' }], big, - { type: 'hbar', x: 0, y: [1], series: null }); - expect(out.labels).toHaveLength(500); + const hbar = buildChartData(bigCols, big, { type: 'hbar', x: 0, y: [1], series: null }); + expect(hbar.labels).toHaveLength(500); // hbar cap + const pie = buildChartData(bigCols, big, { type: 'pie', x: 0, y: [1], series: null }); + expect(pie.labels).toHaveLength(30); // pie's much tighter legibility cap + const line = buildChartData(bigCols, big, { type: 'line', x: 0, y: [1], series: null }); + expect(line.labels).toHaveLength(600); // line cap (5000) exceeds the row count — no truncation }); it('aggregates (sums) rows sharing an X bucket — single-series path', () => { // two rows for the same carrier are summed, not last-write-wins diff --git a/tests/unit/results.test.js b/tests/unit/results.test.js index 1936683..e95b697 100644 --- a/tests/unit/results.test.js +++ b/tests/unit/results.test.js @@ -2,7 +2,7 @@ import { describe, it, expect, vi } from 'vitest'; import { renderResults, renderJson, renderTable, renderChart, colResizeWidth, openCellDetail, openRowsViewer, installChartZoomFix, visCap, expandDataPane } from '../../src/ui/results.js'; import { makeApp } from '../helpers/fake-app.js'; import { newResult } from '../../src/core/stream.js'; -import { schemaKey } from '../../src/core/chart-data.js'; +import { schemaKey, chartRowCap } from '../../src/core/chart-data.js'; const click = (el) => el.dispatchEvent(new Event('click', { bubbles: true })); @@ -776,6 +776,27 @@ describe('renderChart', () => { renderResults(small); expect(small.dom.resultsRegion.querySelector('.chart-cap-note')).toBeNull(); }); + it('switching chart type re-slices to the new type\'s cap and updates the note', () => { + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = Array.from({ length: 600 }, (_, i) => ['k' + i, String(i)]); + r.progress = { rows: 600, bytes: 100, elapsed_ns: 5e6 }; + const app = appWithResult(r, { resultView: 'chart' }); + renderResults(app); + // default (hbar, autoChart's categorical pick) cap is 500 < 600 rows + expect(app.activeTab().chartCfg.type).toBe('hbar'); + expect(app.dom.resultsRegion.querySelector('.chart-cap-note').textContent) + .toBe('first ' + chartRowCap('hbar') + ' of 600 rows'); + expect(app.chart.config.data.labels).toHaveLength(chartRowCap('hbar')); + // switch to pie: a much tighter legibility cap — re-slices and the note shrinks with it + change(fieldSel(app.dom.resultsRegion, 'Type'), 'pie'); + expect(app.dom.resultsRegion.querySelector('.chart-cap-note').textContent).toContain('first ' + chartRowCap('pie') + ' of'); + expect(app.chart.config.data.labels).toHaveLength(chartRowCap('pie')); + // switch to line: its cap (5000) exceeds the row count — no truncation, no note at all + change(fieldSel(app.dom.resultsRegion, 'Type'), 'line'); + expect(app.dom.resultsRegion.querySelector('.chart-cap-note')).toBeNull(); + expect(app.chart.config.data.labels).toHaveLength(600); + }); it('destroys the previous Chart instance on re-render, and re-derives config on a new schema', () => { const app = appWithResult(chartResult(), { resultView: 'chart' }); renderResults(app);