From 06fa5e15f2798e727d6f07081005903eab4350fd Mon Sep 17 00:00:00 2001 From: Brandon Pereira Date: Mon, 10 Aug 2026 13:36:56 -0600 Subject: [PATCH 1/5] perf(dashboard): cap raw-SQL tile query rows to speed up load on high-cardinality tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raw SQL dashboard tiles rendered as a line, stacked-bar, pie, or bar chart had no bound on how many rows a query could return. A pathological high-cardinality group-by could stream hundreds of thousands of rows into the browser, and the client had to transform and materialize all of them before drawing — the dominant cost of loading a heavy dashboard. Builds on the per-tile series cap (#2802), which bounds what is *rendered*; this bounds what is *transferred and materialized*. Fewer rows returned = less client-side processing = faster tile load. Cap raw-SQL tile queries at 5,000 rows (+1 headroom) via two complementary ClickHouse settings: - max_result_rows + result_overflow_mode='break' bounds result rows - max_rows_to_group_by + group_by_overflow_mode='any' bounds aggregation cardinality (the setting that actually protects memory, since 'break' alone is defeated by a result that fits in one block) The +1 headroom means a complete result of exactly the cap comes back whole and is not flagged; only a larger result trips the cap. Detection uses the returned row count alone (never rows_before_limit_at_least), so a tile whose own SQL ends in a LIMIT is not falsely flagged. Builder tiles are unaffected (they already bound cardinality via the series limit; builder tables page rows separately). When a query hits the cap, an inline banner below the chart header notes the chart may be missing data and nudges the user to narrow the query. The banner clears while a narrowed query is in flight, and when a result is both row-capped and series-capped the hidden-series notice no longer claims all series were loaded. --- .changeset/dashboard-tile-result-row-cap.md | 30 +++ packages/app/src/DBDashboardPage.tsx | 15 ++ packages/app/src/__tests__/defaults.test.ts | 74 +++++++ packages/app/src/components/DBBarChart.tsx | 20 +- packages/app/src/components/DBPieChart.tsx | 20 +- packages/app/src/components/DBTimeChart.tsx | 43 ++++- .../components/charts/CategoricalChart.tsx | 33 +++- .../src/components/charts/ChartContainer.tsx | 16 +- .../charts/HiddenSeriesIndicator.tsx | 51 ++++- .../charts/ResultOverflowBanner.tsx | 93 +++++++++ .../__tests__/ResultOverflowBanner.test.tsx | 45 +++++ packages/app/src/defaults.ts | 78 ++++++++ .../hooks/__tests__/useChartConfig.test.tsx | 182 ++++++++++++++++++ packages/app/src/hooks/useChartConfig.tsx | 61 +++++- 14 files changed, 743 insertions(+), 18 deletions(-) create mode 100644 .changeset/dashboard-tile-result-row-cap.md create mode 100644 packages/app/src/__tests__/defaults.test.ts create mode 100644 packages/app/src/components/charts/ResultOverflowBanner.tsx create mode 100644 packages/app/src/components/charts/__tests__/ResultOverflowBanner.test.tsx diff --git a/.changeset/dashboard-tile-result-row-cap.md b/.changeset/dashboard-tile-result-row-cap.md new file mode 100644 index 0000000000..b98fd9428d --- /dev/null +++ b/.changeset/dashboard-tile-result-row-cap.md @@ -0,0 +1,30 @@ +--- +'@hyperdx/app': minor +--- + +Cap the number of rows a raw SQL dashboard tile query returns to protect the +browser from pathological high-cardinality queries. Raw SQL tiles rendered as a +line, stacked-bar, pie, or bar chart now run with two complementary ClickHouse +guards capped at 5,000 (+1 row of headroom): `max_result_rows` + +`result_overflow_mode = 'break'` (bounds the result rows) and +`max_rows_to_group_by` + `group_by_overflow_mode = 'any'` (bounds the aggregation +cardinality — the number of unique GROUP BY keys — which is what actually +protects server memory, since `break` alone is defeated by results that fit in a +single block). (Builder tiles already bound cardinality via the per-tile series +limit, and builder tables page rows separately, so they are unaffected.) When a +query hits the cap, an inline warning below the chart header notes the chart may +be missing data and nudges the user to narrow the query. + +To avoid a false warning when a complete result happens to be exactly the cap +size, the query runs with one row of headroom (`cap + 1`): a result of ≤ cap +rows comes back whole and is not flagged, while a larger result trips the cap and +is detected via the returned row count. Only the returned row count is used for +detection (never `rows_before_limit_at_least`), so a tile whose own SQL ends in a +`LIMIT` is not falsely flagged just because the pre-`LIMIT` aggregation was +large. Both caps are block-aligned (soft), so the returned result can overshoot +the cap by up to one block; because detection can't prove the server actually +dropped rows, the banner says the chart "may be missing data" rather than +asserting truncation. The warning also clears while a narrowed query is +in flight (it no longer lingers on stale placeholder data), and when a chart's +result is both row-capped and series-capped the "hidden series" notice no longer +claims all series were loaded. diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index d82c11217d..f91e82c472 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -144,6 +144,7 @@ import { useDashboards, useDeleteDashboard, } from '@/dashboard'; +import { DEFAULT_MAX_TILE_RESULT_ROWS } from '@/defaults'; import { useAlertAnnotations } from '@/hooks/useAlertAnnotations'; import useDashboardContainers, { TabDeleteAction, @@ -1100,6 +1101,17 @@ const Tile = forwardRef( } : undefined; + // The row cap only applies to raw SQL tiles. Builder (line/bar/pie) + // tiles already bound cardinality via the per-tile series limit, and + // builder tables page rows through useOffsetPaginatedQuery. Raw SQL + // tiles rendered as a time/categorical chart have no such guard, so a + // pathological query can stream an unbounded result into the browser — + // this caps it and lets the tile surface an overflow banner. + const tileMaxResultRows = + effectiveQueriedConfig && isRawSqlChartConfig(effectiveQueriedConfig) + ? DEFAULT_MAX_TILE_RESULT_ROWS + : undefined; + // Markdown charts may not have queriedConfig, if config.source is not set const effectiveMarkdownConfig = effectiveQueriedConfig ?? chart.config; @@ -1145,6 +1157,7 @@ const Tile = forwardRef( enabled={chartEnabled} config={effectiveQueriedConfig} annotations={alertAnnotations} + maxResultRows={tileMaxResultRows} onTimeRangeSelect={ readOnly ? undefined @@ -1209,6 +1222,7 @@ const Tile = forwardRef( toolbarSuffix={toolbarSuffixItems} enabled={chartEnabled} config={effectiveQueriedConfig} + maxResultRows={tileMaxResultRows} /> )} {effectiveQueriedConfig?.displayType === DisplayType.Bar && ( @@ -1219,6 +1233,7 @@ const Tile = forwardRef( toolbarSuffix={toolbarSuffixItems} enabled={chartEnabled} config={effectiveQueriedConfig} + maxResultRows={tileMaxResultRows} /> )} {effectiveQueriedConfig?.displayType === DisplayType.Heatmap && diff --git a/packages/app/src/__tests__/defaults.test.ts b/packages/app/src/__tests__/defaults.test.ts new file mode 100644 index 0000000000..132e0f919d --- /dev/null +++ b/packages/app/src/__tests__/defaults.test.ts @@ -0,0 +1,74 @@ +import { + DEFAULT_MAX_TILE_RESULT_ROWS, + didResultOverflow, + resolveResultRowLimitSetting, +} from '@/defaults'; + +describe('resolveResultRowLimitSetting', () => { + it('requests one row of headroom above the cap', () => { + // cap + 1 lets a complete result of exactly `cap` rows come back whole, + // while anything larger trips the break. + expect(resolveResultRowLimitSetting(5000)).toBe(5001); + expect(resolveResultRowLimitSetting(1)).toBe(2); + }); + + it('floors non-integer caps before adding headroom', () => { + expect(resolveResultRowLimitSetting(99.9)).toBe(100); + }); + + it('returns undefined for a non-positive / absent cap (no limit applied)', () => { + expect(resolveResultRowLimitSetting(undefined)).toBeUndefined(); + expect(resolveResultRowLimitSetting(0)).toBeUndefined(); + expect(resolveResultRowLimitSetting(-10)).toBeUndefined(); + expect( + resolveResultRowLimitSetting(Number.POSITIVE_INFINITY), + ).toBeUndefined(); + }); +}); + +describe('didResultOverflow', () => { + const cap = 5000; + + it('returns false when no cap is applied', () => { + expect(didResultOverflow({ rows: 999999, cap: undefined })).toBe(false); + expect(didResultOverflow({ rows: 999999, cap: 0 })).toBe(false); + expect( + didResultOverflow({ rows: 999999, cap: Number.POSITIVE_INFINITY }), + ).toBe(false); + }); + + it('does NOT flag a complete result of exactly the cap (no truncation)', () => { + // With cap + 1 headroom, a real result of exactly `cap` rows comes back + // whole — nothing was dropped, so no banner. + expect(didResultOverflow({ rows: cap, cap })).toBe(false); + }); + + it('detects overflow when rows exceed the cap', () => { + // break returns cap + 1 (block-aligned, possibly more) once the underlying + // result is larger than the cap. + expect(didResultOverflow({ rows: cap + 1, cap })).toBe(true); + expect(didResultOverflow({ rows: cap + 500, cap })).toBe(true); + }); + + it('does NOT flag when the query has its own LIMIT that trims a large aggregation', () => { + // Regression: a raw SQL tile ending in `... LIMIT 50` over a 6,250-group + // aggregation returns exactly 50 rows. ClickHouse reports + // rows_before_limit_at_least = 6250, but the tile received only 50 rows — + // nothing was truncated by our cap, so there must be no banner. We rely on + // the returned `rows` only, so this is inherently safe. + expect(didResultOverflow({ rows: 50, cap })).toBe(false); + }); + + it('returns false for a normal under-cap result', () => { + expect(didResultOverflow({ rows: 42, cap })).toBe(false); + expect(didResultOverflow({ rows: 0, cap })).toBe(false); + }); + + it('handles a missing row count gracefully', () => { + expect(didResultOverflow({ rows: undefined, cap })).toBe(false); + }); + + it('exposes a sane default cap', () => { + expect(DEFAULT_MAX_TILE_RESULT_ROWS).toBe(5000); + }); +}); diff --git a/packages/app/src/components/DBBarChart.tsx b/packages/app/src/components/DBBarChart.tsx index c95bdd5064..260799bf2e 100644 --- a/packages/app/src/components/DBBarChart.tsx +++ b/packages/app/src/components/DBBarChart.tsx @@ -20,6 +20,7 @@ import { import ChartContainer from './charts/ChartContainer'; import ChartErrorState from './charts/ChartErrorState'; import { ChartTooltipContainer, ChartTooltipItem } from './charts/ChartTooltip'; +import ResultOverflowBanner from './charts/ResultOverflowBanner'; const MAX_BAR_LABEL_LENGTH = 14; const BAR_LABEL_AXIS_HEIGHT = 80; // increased height to accommodate rotated + truncated labels @@ -68,10 +69,27 @@ export const DBBarChart = (props: CategoricalChartProps) => { error, chartData, responseFormatError, + didOverflow, + maxResultRows, + rows, + series, } = useCategoricalChart(props); return ( - + + ) : undefined + } + > {isLoading && !data ? (
Loading Chart Data... diff --git a/packages/app/src/components/DBPieChart.tsx b/packages/app/src/components/DBPieChart.tsx index a0eba392a0..ada6921335 100644 --- a/packages/app/src/components/DBPieChart.tsx +++ b/packages/app/src/components/DBPieChart.tsx @@ -12,6 +12,7 @@ import { import ChartContainer from './charts/ChartContainer'; import ChartErrorState from './charts/ChartErrorState'; import { ChartTooltipContainer, ChartTooltipItem } from './charts/ChartTooltip'; +import ResultOverflowBanner from './charts/ResultOverflowBanner'; const PieChartTooltip = memo( ({ @@ -98,10 +99,27 @@ export const DBPieChart = (props: CategoricalChartProps) => { error, chartData, responseFormatError, + didOverflow, + maxResultRows, + rows, + series, } = useCategoricalChart(props); return ( - + + ) : undefined + } + > {isLoading && !data ? (
Loading Chart Data... diff --git a/packages/app/src/components/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart.tsx index 99cc456b75..8ab5d479c1 100644 --- a/packages/app/src/components/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart.tsx @@ -56,6 +56,7 @@ import ChartErrorState, { import DateRangeIndicator from './charts/DateRangeIndicator'; import DisplaySwitcher from './charts/DisplaySwitcher'; import HiddenSeriesIndicator from './charts/HiddenSeriesIndicator'; +import ResultOverflowBanner from './charts/ResultOverflowBanner'; import MVOptimizationIndicator from './MaterializedViews/MVOptimizationIndicator'; /** A single group column / value pair decoded from a chart series key. */ @@ -323,6 +324,13 @@ type DBTimeChartComponentProps = { * behavior), which is all a standalone chart can do. */ onFocusSeries?: (filters: SeriesGroupFilter[]) => void; + /** + * Caps the number of rows the underlying query returns (see + * useQueriedChartConfig). Used by dashboard tiles to guard against runaway + * high-cardinality queries. When set and the cap is exceeded, an overflow + * banner is rendered below the chart header. + */ + maxResultRows?: number; }; function DBTimeChartComponent({ @@ -348,6 +356,7 @@ function DBTimeChartComponent({ showDateRangeIndicator = true, errorVariant, onFocusSeries, + maxResultRows, }: DBTimeChartComponentProps) { const [selectedSeriesSet, setSelectedSeriesSet] = useState>( new Set(), @@ -486,8 +495,20 @@ function DBTimeChartComponent({ enableQueryChunking: !disableQueryChunking, enableParallelQueries: enableParallelQueries && me?.team?.parallelizeWhenPossible, + maxResultRows, }); + // Whether the query exceeded the row cap. Gated on completion so the banner + // doesn't flap mid-stream while chunks accumulate toward the cap, AND on + // freshness (!isPlaceholderData) so a stale "Result truncated" banner from a + // prior query doesn't linger while the user's narrowed query is in flight — + // otherwise narrowing to escape the cap reads as "my fix didn't work" until + // the new result settles. + const didOverflow = + !isPlaceholderData && data?.isComplete + ? (data?.didOverflow ?? false) + : false; + const previousPeriodChartConfig: ChartConfigWithDateRange = useMemo(() => { const previousPeriodDateRange = queriedConfig.alignDateRangeToGranularity === false @@ -911,6 +932,11 @@ function DBTimeChartComponent({ key="db-time-chart-hidden-series-indicator" hiddenSeriesCount={hiddenSeriesCount} renderedSeriesCount={renderedSeriesCount} + rowCount={data?.rows} + // When the row cap truncated the result, the series counts describe + // only the capped subset — tell the indicator not to claim "all + // series were loaded", which would contradict the overflow banner. + resultWasCapped={didOverflow} // Offered only while still capped AND when load-all would actually // raise the cap (loadAllHandler is undefined otherwise), so the // notice never advertises a no-op click. @@ -939,11 +965,26 @@ function DBTimeChartComponent({ queriedConfig, hiddenSeriesCount, renderedSeriesCount, + data?.rows, + didOverflow, loadAllHandler, ]); return ( - + + ) : undefined + } + > {isLoading && !data ? (
Loading Chart Data... diff --git a/packages/app/src/components/charts/CategoricalChart.tsx b/packages/app/src/components/charts/CategoricalChart.tsx index 936af713cd..42831812eb 100644 --- a/packages/app/src/components/charts/CategoricalChart.tsx +++ b/packages/app/src/components/charts/CategoricalChart.tsx @@ -26,6 +26,13 @@ export interface CategoricalChartProps { toolbarPrefix?: React.ReactNode[]; toolbarSuffix?: React.ReactNode[]; errorVariant?: ChartErrorStateVariant; + /** + * Caps the number of rows the underlying query returns (see + * useQueriedChartConfig). Used by dashboard tiles to guard against runaway + * high-cardinality queries. When set and the cap is exceeded, an overflow + * banner is rendered below the chart header. + */ + maxResultRows?: number; } /** @@ -39,6 +46,7 @@ export function useCategoricalChart({ showMVOptimizationIndicator = true, toolbarPrefix, toolbarSuffix, + maxResultRows, }: CategoricalChartProps) { const { data: source } = useSource({ id: config.source }); @@ -56,14 +64,22 @@ export function useCategoricalChart({ const { data: mvOptimizationData } = useMVOptimizationExplanation(builderQueriedConfig); - const { data, isLoading, isError, error } = useQueriedChartConfig( - queriedConfig, - { + const { data, isLoading, isError, error, isPlaceholderData } = + useQueriedChartConfig(queriedConfig, { placeholderData: (prev: any) => prev, queryKey: [queryKeyPrefix, queriedConfig], enabled, - }, - ); + maxResultRows, + }); + + // Whether the query exceeded the row cap. Gated on completion (no mid-stream + // flap) AND freshness (!isPlaceholderData) so a stale "Result truncated" + // banner from a prior query doesn't linger while a narrowed query is in + // flight. See the matching gate in DBTimeChart. + const didOverflow = + !isPlaceholderData && data?.isComplete + ? (data?.didOverflow ?? false) + : false; const toolbarItems = useMemo(() => { const allToolbarItems: React.ReactNode[] = []; @@ -134,5 +150,12 @@ export function useCategoricalChart({ error, chartData, responseFormatError, + didOverflow, + maxResultRows, + // Row count and category (series) count of the (capped) result, surfaced so + // the overflow banner can report concrete sizes. For a categorical chart the + // number of slices/bars is the series count. + rows: data?.rows, + series: chartData.length, }; } diff --git a/packages/app/src/components/charts/ChartContainer.tsx b/packages/app/src/components/charts/ChartContainer.tsx index d92ad1c168..c9ccd5ba67 100644 --- a/packages/app/src/components/charts/ChartContainer.tsx +++ b/packages/app/src/components/charts/ChartContainer.tsx @@ -9,6 +9,12 @@ interface ChartContainerProps { toolbarItems?: React.ReactNode[]; children: React.ReactNode; disableReactiveContainer?: boolean; + /** + * Optional content rendered directly below the header row and above the chart + * body — e.g. a per-tile warning banner. Takes its natural height; the chart + * body flexes to fill the remaining space. + */ + belowHeader?: React.ReactNode; } // When true, ChartContainer renders a "card" style header: an inline header @@ -71,6 +77,7 @@ function ChartContainer({ toolbarItems, children, disableReactiveContainer, + belowHeader, }: ChartContainerProps) { const cardHeader = use(ChartContainerCardHeaderContext); const collapsedToolbar = use(CollapsedToolbarContext); @@ -185,6 +192,7 @@ function ChartContainer({ )} )} + {belowHeader} {disableReactiveContainer ? ( children ) : ( @@ -193,7 +201,13 @@ function ChartContainer({ style={{ position: 'relative', width: '100%', - height: '100%', + // Flex-fill the remaining height (after the header and any + // belowHeader banner) instead of a rigid 100%, so a banner + // squishes the chart rather than clipping its bottom. minHeight:0 + // lets the absolutely-positioned inner chart shrink below its + // intrinsic size. + flex: 1, + minHeight: 0, }} >
void; } /** - * Warns that the chart returned more series than the client renders. The - * transform caps series to protect memory; this surfaces the dropped ones and, - * when `onLoadAll` is provided, lets the user render all of them anyway. + * Client-side SERIES cap notice. The loaded result draws only the top-N series + * (by peak value) to keep the page responsive — this surfaces how many were + * hidden and, when `onLoadAll` is provided, lets the user draw all of them + * anyway. Normally the RECOVERABLE case: all fetched data is present, it's just + * not all drawn. Contrast ResultOverflowBanner, which reports that the query may + * not have fetched everything (row cap). When `resultWasCapped` is true both + * conditions hold at once, so the copy drops the "all series were loaded" claim + * to avoid contradicting the overflow banner. Copy deliberately uses "drawn" + * here vs the row-cap banner's "missing data" so the two stay distinct. */ export default function HiddenSeriesIndicator({ hiddenSeriesCount, renderedSeriesCount, + rowCount, + resultWasCapped, onLoadAll, }: HiddenSeriesIndicatorProps) { if (hiddenSeriesCount <= 0) { @@ -23,13 +44,27 @@ export default function HiddenSeriesIndicator({ } const total = renderedSeriesCount + hiddenSeriesCount; + // Row count is secondary detail (series is the primary unit for this notice). + const rowsDetail = + typeof rowCount === 'number' + ? ` (from ${rowCount.toLocaleString()} rows)` + : ''; + // When the result was capped, the loaded data is already a subset — don't + // claim "all series were loaded" (that's the overflow banner's job to + // qualify). Otherwise state completeness so the recoverable case is clear. + const completenessClause = resultWasCapped + ? `Of the ${total.toLocaleString()} series in the loaded (capped) result, ` + + `only the ${renderedSeriesCount.toLocaleString()} largest (by peak value) ` + + `are drawn` + : `All ${total.toLocaleString()} series were loaded${rowsDetail}, but only ` + + `the ${renderedSeriesCount.toLocaleString()} largest (by peak value) are drawn`; const label = - `This query returned ${total.toLocaleString()} series. ` + - `${hiddenSeriesCount.toLocaleString()} low-value series were hidden to keep the page responsive; ` + - `showing the top ${renderedSeriesCount.toLocaleString()} by peak value. ` + + `Showing top ${renderedSeriesCount.toLocaleString()} of ` + + `${total.toLocaleString()} series. ${completenessClause} to keep the chart ` + + `readable — ${hiddenSeriesCount.toLocaleString()} smaller series are hidden. ` + (onLoadAll - ? `Click to load all ${total.toLocaleString()} (may be slow).` - : 'Add a stricter GROUP BY, a WHERE filter, or a series limit to reduce cardinality.'); + ? `Click to draw all ${total.toLocaleString()} (may be slow).` + : 'Increase the series limit, or narrow the query with a stricter GROUP BY or WHERE filter, to see more.'); const icon = ( diff --git a/packages/app/src/components/charts/ResultOverflowBanner.tsx b/packages/app/src/components/charts/ResultOverflowBanner.tsx new file mode 100644 index 0000000000..f6e62ee523 --- /dev/null +++ b/packages/app/src/components/charts/ResultOverflowBanner.tsx @@ -0,0 +1,93 @@ +import { Alert, Text, Tooltip } from '@mantine/core'; +import { IconAlertTriangle } from '@tabler/icons-react'; + +interface ResultOverflowBannerProps { + /** Whether the query hit the row cap. When false, nothing renders. */ + didOverflow: boolean | undefined; + /** The row cap that was applied, shown in the message. */ + cap: number; + /** + * Rows actually returned by the (capped) query — shown so the user sees the + * concrete size. Because the cap is block-aligned this is ~cap, not the true + * uncapped total. Omit if unknown. + */ + rows?: number; + /** + * Distinct series (groups) present in the capped result — shown alongside + * rows so this banner is directly comparable to the hidden-series indicator. + * Omit if unknown. + */ + series?: number; +} + +/** + * Server-side ROW cap banner, rendered just below a chart's header (via + * ChartContainer's `belowHeader` slot) when the tile's query hit the row / + * group-by cap (see DEFAULT_MAX_TILE_RESULT_ROWS). Because detection is based on + * the returned row count alone (`rows > cap`), it cannot prove the server + * actually dropped rows — a block-aligned break or a non-aggregating query can + * legitimately return a complete result that merely exceeds the cap. So the copy + * says the chart *may be missing data* rather than asserting truncation. The fix + * is always the same: narrow the query. Contrast HiddenSeriesIndicator, which + * loads the full result and only limits how many series are *drawn* (recoverable + * via "load all"); this banner is about data the query may not have fetched at + * all. Kept to one small line so it doesn't crowd the chart; the full guidance + * lives in the tooltip. + */ +export default function ResultOverflowBanner({ + didOverflow, + cap, + rows, + series, +}: ResultOverflowBannerProps) { + if (!didOverflow) { + return null; + } + + // Rows is the primary unit for this banner; prefer the concrete returned-row + // count, fall back to the cap. Both are approximate (the cap is block-aligned), + // so always prefix with "~". Series is secondary detail. + const rowsShown = (rows ?? cap).toLocaleString(); + const seriesText = + typeof series === 'number' ? ` (~${series.toLocaleString()} series)` : ''; + + return ( + + } + py={4} + px="xs" + styles={{ + // Keep the alert to a single tight line regardless of the tile body's + // fs-7 / monospace context; the chart gets the remaining height. + wrapper: { alignItems: 'center' }, + body: { minWidth: 0 }, + message: { margin: 0 }, + icon: { marginRight: 8, width: 14, height: 14 }, + }} + // Sits inside react-grid-layout's drag subtree; stop propagation so a + // click on the banner doesn't start a tile drag. + onMouseDown={e => e.stopPropagation()} + > + + Hit the ~{rowsShown}-row cap{seriesText} — chart may be missing data. + + + + ); +} diff --git a/packages/app/src/components/charts/__tests__/ResultOverflowBanner.test.tsx b/packages/app/src/components/charts/__tests__/ResultOverflowBanner.test.tsx new file mode 100644 index 0000000000..f8f68b67ba --- /dev/null +++ b/packages/app/src/components/charts/__tests__/ResultOverflowBanner.test.tsx @@ -0,0 +1,45 @@ +import { screen } from '@testing-library/react'; + +import ResultOverflowBanner from '@/components/charts/ResultOverflowBanner'; + +describe('ResultOverflowBanner', () => { + it('renders nothing when didOverflow is false', () => { + renderWithMantine(); + expect(screen.queryByText(/row cap/)).not.toBeInTheDocument(); + }); + + it('renders nothing when didOverflow is undefined', () => { + renderWithMantine( + , + ); + expect(screen.queryByText(/row cap/)).not.toBeInTheDocument(); + }); + + it('renders the cap warning with the returned row count when overflowed', () => { + renderWithMantine( + , + ); + // Prefers the concrete returned-row count over the cap. + expect(screen.getByText(/6,250-row cap/)).toBeInTheDocument(); + expect(screen.getByText(/may be missing data/)).toBeInTheDocument(); + }); + + it('falls back to the cap when rows is omitted', () => { + renderWithMantine(); + expect(screen.getByText(/5,000-row cap/)).toBeInTheDocument(); + }); + + it('includes the series count when provided', () => { + renderWithMantine( + , + ); + expect(screen.getByText(/~1,250 series/)).toBeInTheDocument(); + }); + + it('omits the series suffix when series is not provided', () => { + renderWithMantine( + , + ); + expect(screen.queryByText(/series/)).not.toBeInTheDocument(); + }); +}); diff --git a/packages/app/src/defaults.ts b/packages/app/src/defaults.ts index 5894d8b78d..a6c212fe10 100644 --- a/packages/app/src/defaults.ts +++ b/packages/app/src/defaults.ts @@ -34,6 +34,84 @@ export const MAX_EXPANDED_TOOLTIP_ROWS = 500; // (HARD_LINES_LIMIT); drawn lines remain bounded by HARD_LINES_LIMIT regardless. export const MAX_LOADABLE_TIME_CHART_SERIES = 5000; +// Soft ceiling on the number of rows a single dashboard-tile query returns. +// This is a last-line defense against a pathological high-cardinality group-by +// streaming hundreds of thousands of rows into the browser — well above the +// series/draw caps, which bound what is *rendered*, whereas this bounds what is +// *transferred* (and materialized server-side). +// +// The cap is enforced by two complementary ClickHouse settings, both asked for +// with one row of HEADROOM above the logical cap (`cap + 1`, see +// `resolveResultRowLimitSetting`) so a complete result of exactly `cap` rows +// comes back whole and is NOT flagged, while a larger result trips the cap: +// 1. `max_result_rows = cap + 1` + `result_overflow_mode = 'break'` — bounds +// the RESULT rows. Weak on its own: break only stops between result blocks, +// so a result that fits in one block (up to ~65k rows) is returned whole. +// 2. `max_rows_to_group_by = cap + 1` + `group_by_overflow_mode = 'any'` — +// bounds the aggregation CARDINALITY (unique GROUP BY keys). "any" keeps the +// first N keys and folds the rest away instead of erroring. This is the +// setting that actually protects memory for a high-cardinality group-by. +// +// IMPORTANT: both are *soft*, block-aligned caps — ClickHouse checks them only +// after each data part, so the real result can overshoot `cap + 1` by up to one +// block. They are NOT exact truncations. When a query returns more than `cap` +// rows we surface an overflow banner on the tile (see `didResultOverflow`) so +// the user knows the result was capped and the chart may be missing data. +export const DEFAULT_MAX_TILE_RESULT_ROWS = 5000; + +/** + * Translate a logical row cap into the `max_result_rows` value to send to + * ClickHouse. We request `cap + 1` so a result of exactly `cap` rows comes back + * whole (and is NOT flagged as overflow), while anything larger trips the break + * and returns at least `cap + 1` rows. Returns undefined for a non-positive / + * absent cap (no limit applied). + */ +export function resolveResultRowLimitSetting( + cap: number | undefined, +): number | undefined { + if (cap == null || !Number.isFinite(cap) || cap <= 0) { + return undefined; + } + return Math.floor(cap) + 1; +} + +/** + * Decide whether a tile query exceeded the `max_result_rows` / + * `result_overflow_mode = 'break'` cap. + * + * The query is run with one row of headroom (`max_result_rows = cap + 1`; see + * `resolveResultRowLimitSetting`), so a complete result of ≤ cap rows returns + * as-is and does NOT flag; only a result larger than `cap` trips the break and + * comes back with more than `cap` rows (≥ cap + 1, possibly more since break is + * block-aligned). Hence the sole reliable signal is `rows > cap`. + * + * We deliberately do NOT use `rows_before_limit_at_least`: ClickHouse only + * populates it when the query itself has a LIMIT stage, and it then reports the + * row count BEFORE that user LIMIT — not before our result-size break. A raw + * SQL tile whose query ends in `... LIMIT 50` over a large aggregation returns + * exactly 50 rows (nothing truncated by us) yet reports + * `rows_before_limit_at_least = `, which would wrongly + * trip the banner. `rows > cap` cannot false-positive that way because the tile + * genuinely received ≤ cap rows. + * + * A non-positive/absent cap disables detection. + */ +export function didResultOverflow({ + rows, + cap, +}: { + rows: number | undefined; + cap: number | undefined; +}): boolean { + if (cap == null || !Number.isFinite(cap) || cap <= 0) { + return false; + } + if (typeof rows === 'number' && rows > cap) { + return true; + } + return false; +} + /** * Resolve the effective client-side render cap from a tile's `seriesLimit` * (see SharedChartSettingsSchema): null/undefined → the default cap, exactly 0 → diff --git a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx index 7b9eab0b7c..31b8c338d0 100644 --- a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx +++ b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx @@ -415,6 +415,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -427,6 +428,87 @@ describe('useChartConfig', () => { expect(result.current.isPending).toBe(false); }); + it('applies max_result_rows + max_rows_to_group_by (cap + 1 headroom) and flags overflow', async () => { + const config = createMockChartConfig({ + dateRange: undefined, + granularity: '1 minute', + }); + + // 2 rows returned against a cap of 1 → rows (2) > cap (1) => overflow. + const mockResponse = createMockQueryResponse([ + { + 'count()': '71', + SeverityText: 'info', + __hdx_time_bucket: '2025-10-01T00:00:00Z', + }, + { + 'count()': '73', + SeverityText: 'warn', + __hdx_time_bucket: '2025-10-01T00:00:00Z', + }, + ]); + mockClickhouseClient.queryChartConfig.mockResolvedValue(mockResponse); + + const { result } = renderHook( + () => useQueriedChartConfig(config, { maxResultRows: 1 }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + // Both the result-row cap and the group-by cardinality cap are sent, each + // with one row of headroom (cap + 1), so the aggregation is genuinely + // bounded (not just the block-aligned result-row break). + expect(mockClickhouseClient.queryChartConfig).toHaveBeenCalledWith({ + config, + metadata: expect.any(Object), + opts: { + abort_signal: expect.any(AbortSignal), + clickhouse_settings: { + max_result_rows: '2', + result_overflow_mode: 'break', + max_rows_to_group_by: '2', + group_by_overflow_mode: 'any', + }, + }, + }); + expect(result.current.data?.didOverflow).toBe(true); + }); + + it('does not cap or flag overflow when maxResultRows is not set', async () => { + const config = createMockChartConfig({ + dateRange: undefined, + granularity: '1 minute', + }); + + const mockResponse = createMockQueryResponse([ + { + 'count()': '71', + SeverityText: 'info', + __hdx_time_bucket: '2025-10-01T00:00:00Z', + }, + ]); + mockClickhouseClient.queryChartConfig.mockResolvedValue(mockResponse); + + const { result } = renderHook(() => useQueriedChartConfig(config), { + wrapper, + }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + expect(mockClickhouseClient.queryChartConfig).toHaveBeenCalledWith({ + config, + metadata: expect.any(Object), + opts: { + abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, + }, + }); + expect(result.current.data?.didOverflow).toBeUndefined(); + }); + it('fetches data without chunking when no granularity is provided', async () => { const config = createMockChartConfig({ dateRange: [new Date('2025-10-01'), new Date('2025-10-02')], @@ -464,6 +546,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -518,6 +601,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -597,6 +681,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -648,6 +733,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -696,6 +782,7 @@ describe('useChartConfig', () => { metadata: expect.any(Object), opts: { abort_signal: expect.any(AbortSignal), + clickhouse_settings: {}, }, }); expect(result.current.data).toEqual({ @@ -802,6 +889,101 @@ describe('useChartConfig', () => { expect(result.current.isPending).toBe(false); }); + it('flags overflow across chunks only when an individual chunk exceeds the cap', async () => { + // Chunked + capped: the cap is applied per chunk, so a single chunk that + // exceeds the cap is what signals a truncated result — even though other + // chunks are small. cap = 2 here. + const config = createMockChartConfig({ + dateRange: [ + new Date('2025-10-01 00:00:00Z'), + new Date('2025-10-02 00:00:00Z'), + ], + granularity: '3 hour', + }); + + // Newest window: 3 rows > cap(2) => this chunk overflowed. + const overflowingChunk = createMockQueryResponse([ + { 'count()': '1', __hdx_time_bucket: '2025-10-01T18:00:00Z' }, + { 'count()': '2', __hdx_time_bucket: '2025-10-01T19:00:00Z' }, + { 'count()': '3', __hdx_time_bucket: '2025-10-01T20:00:00Z' }, + ]); + // Older windows: each <= cap. + const smallChunkA = createMockQueryResponse([ + { 'count()': '4', __hdx_time_bucket: '2025-10-01T12:00:00Z' }, + ]); + const smallChunkB = createMockQueryResponse([ + { 'count()': '5', __hdx_time_bucket: '2025-10-01T01:00:00Z' }, + ]); + + mockClickhouseClient.queryChartConfig + .mockResolvedValueOnce(overflowingChunk) + .mockResolvedValueOnce(smallChunkA) + .mockResolvedValueOnce(smallChunkB); + + const { result } = renderHook( + () => + useQueriedChartConfig(config, { + enableQueryChunking: true, + maxResultRows: 2, + }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + expect(result.current.data?.didOverflow).toBe(true); + expect(result.current.data?.isComplete).toBe(true); + }); + + it('does NOT flag overflow when the summed multi-chunk total exceeds the cap but no single chunk does', async () => { + // Regression guard: each chunk is individually within the cap, so nothing + // was truncated — the accumulated sum (which legitimately exceeds the cap) + // must NOT trip the banner. cap = 2, three chunks of <= 2 rows summing to 5. + const config = createMockChartConfig({ + dateRange: [ + new Date('2025-10-01 00:00:00Z'), + new Date('2025-10-02 00:00:00Z'), + ], + granularity: '3 hour', + }); + + const chunk1 = createMockQueryResponse([ + { 'count()': '1', __hdx_time_bucket: '2025-10-01T18:00:00Z' }, + { 'count()': '2', __hdx_time_bucket: '2025-10-01T19:00:00Z' }, + ]); + const chunk2 = createMockQueryResponse([ + { 'count()': '3', __hdx_time_bucket: '2025-10-01T12:00:00Z' }, + { 'count()': '4', __hdx_time_bucket: '2025-10-01T14:00:00Z' }, + ]); + const chunk3 = createMockQueryResponse([ + { 'count()': '5', __hdx_time_bucket: '2025-10-01T01:00:00Z' }, + ]); + + mockClickhouseClient.queryChartConfig + .mockResolvedValueOnce(chunk1) + .mockResolvedValueOnce(chunk2) + .mockResolvedValueOnce(chunk3); + + const { result } = renderHook( + () => + useQueriedChartConfig(config, { + enableQueryChunking: true, + maxResultRows: 2, + }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + // Summed rows (5) exceed cap (2), but no chunk individually did, so this + // is a complete result — didOverflow must be false. + expect(result.current.data?.rows).toBe(5); + expect(result.current.data?.didOverflow).toBe(false); + expect(result.current.data?.isComplete).toBe(true); + }); + it('pins the series-limit ranking to the newest window on each chunk', async () => { const config = createMockChartConfig({ dateRange: [ diff --git a/packages/app/src/hooks/useChartConfig.tsx b/packages/app/src/hooks/useChartConfig.tsx index bdb5a39f3a..e8740ca406 100644 --- a/packages/app/src/hooks/useChartConfig.tsx +++ b/packages/app/src/hooks/useChartConfig.tsx @@ -37,6 +37,7 @@ import { prometheusApi } from '@/api'; import { toStartOfInterval } from '@/ChartUtils'; import { useClickhouseClient } from '@/clickhouse'; import { IS_MTVIEWS_ENABLED } from '@/config'; +import { didResultOverflow, resolveResultRowLimitSetting } from '@/defaults'; import { buildMTViewSelectQuery } from '@/hdxMTViews'; import { useMetadataWithSettings } from '@/hooks/useMetadata'; import { useSource } from '@/source'; @@ -54,6 +55,15 @@ interface AdditionalUseQueriedChartConfigOptions { */ enableQueryChunking?: boolean; enableParallelQueries?: boolean; + /** + * When set to a positive number, caps the number of rows the query is allowed + * to return via the ClickHouse `max_result_rows` + `result_overflow_mode = + * 'break'` settings, and reports whether the cap was hit via + * `data.didOverflow`. Used by dashboard tiles as a last-line guard against a + * high-cardinality query streaming an unbounded number of rows into the + * browser. Omit (or pass 0/undefined) to leave the query uncapped. + */ + maxResultRows?: number; } type TimeWindow = { @@ -63,6 +73,11 @@ type TimeWindow = { type TQueryFnData = Pick, 'data' | 'meta' | 'rows'> & { isComplete: boolean; + /** + * True when the query exceeded the `maxResultRows` cap and the returned rows + * are a truncated slice of a larger result. Undefined when no cap was applied. + */ + didOverflow?: boolean; }; type TChunk = { @@ -142,6 +157,7 @@ async function* fetchDataInChunks({ enableParallelQueries = false, metadata, querySettings, + maxResultRows, }: { config: ChartConfigWithOptDateRange; clickhouseClient: ClickhouseClient; @@ -150,6 +166,7 @@ async function* fetchDataInChunks({ enableParallelQueries?: boolean; metadata: Metadata; querySettings: QuerySettings | undefined; + maxResultRows?: number; }) { const windows = enableQueryChunking && shouldUseChunking(config) @@ -189,10 +206,36 @@ async function* fetchDataInChunks({ } // Readonly = 2 means the query is readonly but can still specify query settings. - const clickHouseSettings = isRawSqlChartConfig(config) + const clickHouseSettings: Record = isRawSqlChartConfig(config) ? { readonly: '2' } : {}; + // Cap the number of rows any single (possibly chunked) query returns. We ask + // ClickHouse for one row of headroom above the logical cap + // (`max_result_rows = cap + 1`, via resolveResultRowLimitSetting) with + // `result_overflow_mode = 'break'`, so a complete result of exactly `cap` + // rows comes back whole (no false overflow warning) while a larger result + // trips the break and returns cap + 1 rows. The caller detects the overflow + // via the returned `rows` count (see didResultOverflow) and warns the user. + // + // `max_result_rows` alone is a WEAK guard: `result_overflow_mode = 'break'` + // only stops between result blocks, so a result that fits in one block (up to + // ~65k rows) is returned whole and nothing is actually capped. We therefore + // ALSO cap the aggregation cardinality with `max_rows_to_group_by` + + // `group_by_overflow_mode = 'any'`, which bounds the number of unique GROUP BY + // keys the query materializes ("any" keeps the first N keys and folds the + // rest away instead of erroring). This is the setting that actually protects + // memory for a high-cardinality group-by. Both are block-aligned, so the real + // result can overshoot the cap by up to one block — detection stays tolerant + // (rows > cap). Only positive finite caps are applied. + const resultRowLimit = resolveResultRowLimitSetting(maxResultRows); + if (resultRowLimit != null) { + clickHouseSettings.max_result_rows = String(resultRowLimit); + clickHouseSettings.result_overflow_mode = 'break'; + clickHouseSettings.max_rows_to_group_by = String(resultRowLimit); + clickHouseSettings.group_by_overflow_mode = 'any'; + } + if (enableParallelQueries) { // fetch in parallel const promises = windows.map(async (w, index) => { @@ -242,6 +285,7 @@ async function* fetchDataInChunks({ metadata, opts: { abort_signal: signal, + clickhouse_settings: clickHouseSettings, }, querySettings, }); @@ -317,6 +361,7 @@ export function useQueriedChartConfig( config, options?.enableQueryChunking ?? false, options?.enableParallelQueries ?? false, + options?.maxResultRows ?? 0, ], // TODO: Replace this with `streamedQuery` when it is no longer experimental. Use 'replace' refetch mode. // https://tanstack.com/query/latest/docs/reference/streamedQuery @@ -430,15 +475,29 @@ export function useQueriedChartConfig( enableParallelQueries: options?.enableParallelQueries, metadata, querySettings: source?.querySettings, + maxResultRows: options?.maxResultRows, }); + // A chunked range applies the cap per chunk, so the accumulated total can + // legitimately exceed the cap without any single chunk overflowing. Track + // whether ANY individual chunk hit the cap (the definitive per-query + // signal) and OR it with the whole-result check below. + const cap = options?.maxResultRows; + let anyChunkOverflowed = false; + let accumulatedChunks: TQueryFnData = emptyValue; for await (const chunk of chunks) { if (context.signal.aborted) { break; } + if (didResultOverflow({ rows: chunk.chunk.rows, cap })) { + anyChunkOverflowed = true; + } + accumulatedChunks = appendChunk(accumulatedChunks, chunk); + accumulatedChunks.didOverflow = + cap == null ? undefined : anyChunkOverflowed; // When refetching, the cache is not updated until all chunks are fetched. if (!isRefetch) { From 32b43b35b8af5284bd1d913664ad400f9bb48ff2 Mon Sep 17 00:00:00 2001 From: Brandon Pereira Date: Mon, 10 Aug 2026 14:49:54 -0600 Subject: [PATCH 2/5] fix(dashboard): make raw-SQL tile row cap correctness-safe (review fixes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Greptile + deep-review feedback on the tile result-row cap: - P1 (silent top-N corruption): group_by_overflow_mode folds/drops GROUP BY keys DURING aggregation, before an outer `ORDER BY … LIMIT N` runs, so a raw-SQL tile of that shape computed its top-N over an arbitrary key subset and rendered silently wrong values with no banner (result stayed <= cap). Now the cardinality cap (max_rows_to_group_by) is applied ONLY when the query has no outer LIMIT (hasOuterLimit heuristic), and uses 'break' (deterministic stop) instead of 'any' (arbitrary fold). The order-preserving max_result_rows cap still always applies. Centralized in resolveResultRowLimitSettings. - P2: thread maxResultRows into the previous-period comparison query so the compareToPreviousPeriod overlay can't bypass the cap. - P2: add tests for the new UI wiring — resultWasCapped copy branch, raw-vs- builder cap selection (resolveTileMaxResultRows), and the settings chosen for raw SQL with/without an outer LIMIT. - P3: extract shared resolveDidOverflow helper (was duplicated across DBTimeChart/CategoricalChart), fix a stale comment, add a NaN row-count guard test. Also trims the verbose explanatory comments added in the first pass. --- .changeset/dashboard-tile-result-row-cap.md | 25 +-- packages/app/src/DBDashboardPage.tsx | 16 +- packages/app/src/__tests__/defaults.test.ts | 162 ++++++++++++++++++ packages/app/src/components/DBTimeChart.tsx | 37 ++-- .../components/charts/CategoricalChart.tsx | 27 ++- .../src/components/charts/ChartContainer.tsx | 14 +- .../charts/HiddenSeriesIndicator.tsx | 35 ++-- .../charts/ResultOverflowBanner.tsx | 37 +--- .../__tests__/HiddenSeriesIndicator.test.tsx | 33 ++++ packages/app/src/defaults.ts | 146 +++++++++++----- .../hooks/__tests__/useChartConfig.test.tsx | 86 +++++++++- packages/app/src/hooks/useChartConfig.tsx | 56 +++--- 12 files changed, 470 insertions(+), 204 deletions(-) diff --git a/.changeset/dashboard-tile-result-row-cap.md b/.changeset/dashboard-tile-result-row-cap.md index b98fd9428d..f6fdca958f 100644 --- a/.changeset/dashboard-tile-result-row-cap.md +++ b/.changeset/dashboard-tile-result-row-cap.md @@ -4,16 +4,21 @@ Cap the number of rows a raw SQL dashboard tile query returns to protect the browser from pathological high-cardinality queries. Raw SQL tiles rendered as a -line, stacked-bar, pie, or bar chart now run with two complementary ClickHouse -guards capped at 5,000 (+1 row of headroom): `max_result_rows` + -`result_overflow_mode = 'break'` (bounds the result rows) and -`max_rows_to_group_by` + `group_by_overflow_mode = 'any'` (bounds the aggregation -cardinality — the number of unique GROUP BY keys — which is what actually -protects server memory, since `break` alone is defeated by results that fit in a -single block). (Builder tiles already bound cardinality via the per-tile series -limit, and builder tables page rows separately, so they are unaffected.) When a -query hits the cap, an inline warning below the chart header notes the chart may -be missing data and nudges the user to narrow the query. +line, stacked-bar, pie, or bar chart now run capped at 5,000 (+1 row of +headroom) via an order-preserving result-row cap (`max_result_rows` + +`result_overflow_mode = 'break'`, which trims the tail after the query's own +ORDER BY/LIMIT). Because that cap is block-aligned and weak on its own (a result +fitting in one block is returned whole), queries with no outer `LIMIT` also get +a group-by cardinality cap (`max_rows_to_group_by` + `group_by_overflow_mode = +'break'`) that bounds unique GROUP BY keys — the setting that actually protects +server memory. The cardinality cap is deliberately **skipped** for raw SQL that +carries its own outer `LIMIT`: it stops accumulating keys during aggregation, +before an outer `ORDER BY … LIMIT N` runs, so applying it there would compute the +top-N over an arbitrary key subset and render silently wrong values. (Builder +tiles already bound cardinality via the per-tile series limit, and builder tables +page rows separately, so they are unaffected.) When a query hits the cap, an +inline warning below the chart header notes the chart may be missing data and +nudges the user to narrow the query. To avoid a false warning when a complete result happens to be exactly the cap size, the query runs with one row of headroom (`cap + 1`): a result of ≤ cap diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index f91e82c472..ca0e425a4f 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -144,7 +144,7 @@ import { useDashboards, useDeleteDashboard, } from '@/dashboard'; -import { DEFAULT_MAX_TILE_RESULT_ROWS } from '@/defaults'; +import { resolveTileMaxResultRows } from '@/defaults'; import { useAlertAnnotations } from '@/hooks/useAlertAnnotations'; import useDashboardContainers, { TabDeleteAction, @@ -1101,16 +1101,10 @@ const Tile = forwardRef( } : undefined; - // The row cap only applies to raw SQL tiles. Builder (line/bar/pie) - // tiles already bound cardinality via the per-tile series limit, and - // builder tables page rows through useOffsetPaginatedQuery. Raw SQL - // tiles rendered as a time/categorical chart have no such guard, so a - // pathological query can stream an unbounded result into the browser — - // this caps it and lets the tile surface an overflow banner. - const tileMaxResultRows = - effectiveQueriedConfig && isRawSqlChartConfig(effectiveQueriedConfig) - ? DEFAULT_MAX_TILE_RESULT_ROWS - : undefined; + // Only raw SQL tiles get a row cap (see resolveTileMaxResultRows). + const tileMaxResultRows = resolveTileMaxResultRows( + effectiveQueriedConfig, + ); // Markdown charts may not have queriedConfig, if config.source is not set const effectiveMarkdownConfig = effectiveQueriedConfig ?? chart.config; diff --git a/packages/app/src/__tests__/defaults.test.ts b/packages/app/src/__tests__/defaults.test.ts index 132e0f919d..09b47738ac 100644 --- a/packages/app/src/__tests__/defaults.test.ts +++ b/packages/app/src/__tests__/defaults.test.ts @@ -1,7 +1,13 @@ +import type { ChartConfigWithOptDateRange } from '@hyperdx/common-utils/dist/types'; + import { DEFAULT_MAX_TILE_RESULT_ROWS, didResultOverflow, + hasOuterLimit, + resolveDidOverflow, resolveResultRowLimitSetting, + resolveResultRowLimitSettings, + resolveTileMaxResultRows, } from '@/defaults'; describe('resolveResultRowLimitSetting', () => { @@ -68,7 +74,163 @@ describe('didResultOverflow', () => { expect(didResultOverflow({ rows: undefined, cap })).toBe(false); }); + it('does not flag a NaN row count', () => { + // NaN > cap is false, so this is correct today; asserted to lock it in. + expect(didResultOverflow({ rows: Number.NaN, cap })).toBe(false); + }); + it('exposes a sane default cap', () => { expect(DEFAULT_MAX_TILE_RESULT_ROWS).toBe(5000); }); }); + +describe('resolveTileMaxResultRows', () => { + const rawSqlConfig = { + configType: 'sql', + sqlTemplate: 'SELECT 1', + connection: 'c', + } as unknown as ChartConfigWithOptDateRange; + const builderConfig = { + connection: 'c', + from: { databaseName: 'default', tableName: 't' }, + select: [{ aggFn: 'count', valueExpression: '' }], + where: '', + groupBy: 'k', + } as unknown as ChartConfigWithOptDateRange; + + it('caps raw SQL tiles at the default', () => { + expect(resolveTileMaxResultRows(rawSqlConfig)).toBe( + DEFAULT_MAX_TILE_RESULT_ROWS, + ); + }); + + it('does NOT cap builder tiles (they bound cardinality elsewhere)', () => { + expect(resolveTileMaxResultRows(builderConfig)).toBeUndefined(); + }); + + it('returns undefined for an absent config', () => { + expect(resolveTileMaxResultRows(undefined)).toBeUndefined(); + expect(resolveTileMaxResultRows(null)).toBeUndefined(); + }); +}); + +describe('hasOuterLimit', () => { + it('detects a trailing LIMIT (with optional offset / comma / semicolon / comment)', () => { + expect(hasOuterLimit('SELECT * FROM t LIMIT 50')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t ORDER BY c DESC LIMIT 50')).toBe( + true, + ); + expect(hasOuterLimit('SELECT * FROM t limit 10')).toBe(true); // case-insensitive + expect(hasOuterLimit('SELECT * FROM t LIMIT 50;')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 \n ')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 10, 50')).toBe(true); // offset,count + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 OFFSET 10')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 -- trailing comment')).toBe( + true, + ); + }); + + it('returns false when there is no outer LIMIT', () => { + expect(hasOuterLimit('SELECT * FROM t')).toBe(false); + expect(hasOuterLimit('SELECT k, count() FROM t GROUP BY k')).toBe(false); + }); + + it('does not treat a LIMIT inside a subquery as an outer LIMIT', () => { + // Anchored to end-of-string: an inner LIMIT followed by more query is not + // matched, so the cardinality cap can still apply. + expect( + hasOuterLimit('SELECT * FROM (SELECT * FROM t LIMIT 10) GROUP BY k'), + ).toBe(false); + }); + + it('returns false for empty / non-string input', () => { + expect(hasOuterLimit('')).toBe(false); + expect(hasOuterLimit(undefined)).toBe(false); + }); +}); + +describe('resolveResultRowLimitSettings', () => { + it('returns undefined for a non-positive / absent cap', () => { + expect(resolveResultRowLimitSettings(undefined)).toBeUndefined(); + expect(resolveResultRowLimitSettings(0)).toBeUndefined(); + expect(resolveResultRowLimitSettings(-1)).toBeUndefined(); + }); + + it('applies both the result-row and cardinality caps when there is no outer LIMIT', () => { + const res = resolveResultRowLimitSettings(5000, { hasOuterLimit: false }); + expect(res).toEqual({ + cardinalityCapApplied: true, + settings: { + max_result_rows: '5001', + result_overflow_mode: 'break', + max_rows_to_group_by: '5001', + // 'break' (deterministic stop), never 'any' (folds keys → corrupts top-N) + group_by_overflow_mode: 'break', + }, + }); + }); + + it('applies only the order-preserving result-row cap when there is an outer LIMIT', () => { + const res = resolveResultRowLimitSettings(5000, { hasOuterLimit: true }); + expect(res).toEqual({ + cardinalityCapApplied: false, + settings: { + max_result_rows: '5001', + result_overflow_mode: 'break', + }, + }); + }); + + it('defaults to applying the cardinality cap when the option is omitted', () => { + expect(resolveResultRowLimitSettings(10)?.cardinalityCapApplied).toBe(true); + }); +}); + +describe('resolveDidOverflow', () => { + it('is false until the result is complete (no mid-stream flap)', () => { + expect( + resolveDidOverflow({ + isPlaceholderData: false, + isComplete: false, + didOverflow: true, + }), + ).toBe(false); + }); + + it('is false while showing stale placeholder data (no lingering banner)', () => { + expect( + resolveDidOverflow({ + isPlaceholderData: true, + isComplete: true, + didOverflow: true, + }), + ).toBe(false); + }); + + it('reflects the result once complete and fresh', () => { + expect( + resolveDidOverflow({ + isPlaceholderData: false, + isComplete: true, + didOverflow: true, + }), + ).toBe(true); + expect( + resolveDidOverflow({ + isPlaceholderData: false, + isComplete: true, + didOverflow: false, + }), + ).toBe(false); + }); + + it('treats an undefined didOverflow as false', () => { + expect( + resolveDidOverflow({ + isPlaceholderData: false, + isComplete: true, + didOverflow: undefined, + }), + ).toBe(false); + }); +}); diff --git a/packages/app/src/components/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart.tsx index 8ab5d479c1..4437b95e14 100644 --- a/packages/app/src/components/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart.tsx @@ -41,6 +41,7 @@ import { ChartSeriesTooltip } from '@/components/charts/ChartSeriesTooltip'; import { useChartTooltipZIndex } from '@/components/charts/ChartTooltip'; import { MAX_LOADABLE_TIME_CHART_SERIES, + resolveDidOverflow, resolveRenderedSeriesCap, } from '@/defaults'; import { type ActiveClickPayload, MemoChart } from '@/HDXMultiSeriesTimeChart'; @@ -324,12 +325,7 @@ type DBTimeChartComponentProps = { * behavior), which is all a standalone chart can do. */ onFocusSeries?: (filters: SeriesGroupFilter[]) => void; - /** - * Caps the number of rows the underlying query returns (see - * useQueriedChartConfig). Used by dashboard tiles to guard against runaway - * high-cardinality queries. When set and the cap is exceeded, an overflow - * banner is rendered below the chart header. - */ + /** Row cap for the query (see useQueriedChartConfig); overflow shows a banner. */ maxResultRows?: number; }; @@ -498,16 +494,13 @@ function DBTimeChartComponent({ maxResultRows, }); - // Whether the query exceeded the row cap. Gated on completion so the banner - // doesn't flap mid-stream while chunks accumulate toward the cap, AND on - // freshness (!isPlaceholderData) so a stale "Result truncated" banner from a - // prior query doesn't linger while the user's narrowed query is in flight — - // otherwise narrowing to escape the cap reads as "my fix didn't work" until - // the new result settles. - const didOverflow = - !isPlaceholderData && data?.isComplete - ? (data?.didOverflow ?? false) - : false; + // Whether the query exceeded the row cap. See resolveDidOverflow for the + // completion + freshness gating (shared with CategoricalChart). + const didOverflow = resolveDidOverflow({ + isPlaceholderData, + isComplete: data?.isComplete, + didOverflow: data?.didOverflow, + }); const previousPeriodChartConfig: ChartConfigWithDateRange = useMemo(() => { const previousPeriodDateRange = @@ -543,6 +536,10 @@ function DBTimeChartComponent({ queryKey: [queryKeyPrefix, previousPeriodChartConfig, 'chunked'], enabled: !!(enabled && config.compareToPreviousPeriod), enableQueryChunking: true, + // Cap the comparison series with the same bound as the primary query so + // the previous-period overlay can't bypass the guard. Banner still keys + // off the primary query only. + maxResultRows, }); const isLoadingOrPlaceholder = @@ -933,13 +930,9 @@ function DBTimeChartComponent({ hiddenSeriesCount={hiddenSeriesCount} renderedSeriesCount={renderedSeriesCount} rowCount={data?.rows} - // When the row cap truncated the result, the series counts describe - // only the capped subset — tell the indicator not to claim "all - // series were loaded", which would contradict the overflow banner. + // When capped, the series counts describe only the subset — don't let + // the indicator claim "all series were loaded". resultWasCapped={didOverflow} - // Offered only while still capped AND when load-all would actually - // raise the cap (loadAllHandler is undefined otherwise), so the - // notice never advertises a no-op click. onLoadAll={loadAllHandler} />, ); diff --git a/packages/app/src/components/charts/CategoricalChart.tsx b/packages/app/src/components/charts/CategoricalChart.tsx index 42831812eb..de7b6e80aa 100644 --- a/packages/app/src/components/charts/CategoricalChart.tsx +++ b/packages/app/src/components/charts/CategoricalChart.tsx @@ -9,6 +9,7 @@ import { formatResponseForCategoricalChart, } from '@/ChartUtils'; import MVOptimizationIndicator from '@/components/MaterializedViews/MVOptimizationIndicator'; +import { resolveDidOverflow } from '@/defaults'; import { useQueriedChartConfig } from '@/hooks/useChartConfig'; import { useMVOptimizationExplanation } from '@/hooks/useMVOptimizationExplanation'; import { useSingleSeriesNumberFormat, useSource } from '@/source'; @@ -26,12 +27,7 @@ export interface CategoricalChartProps { toolbarPrefix?: React.ReactNode[]; toolbarSuffix?: React.ReactNode[]; errorVariant?: ChartErrorStateVariant; - /** - * Caps the number of rows the underlying query returns (see - * useQueriedChartConfig). Used by dashboard tiles to guard against runaway - * high-cardinality queries. When set and the cap is exceeded, an overflow - * banner is rendered below the chart header. - */ + /** Row cap for the query (see useQueriedChartConfig); overflow shows a banner. */ maxResultRows?: number; } @@ -72,14 +68,13 @@ export function useCategoricalChart({ maxResultRows, }); - // Whether the query exceeded the row cap. Gated on completion (no mid-stream - // flap) AND freshness (!isPlaceholderData) so a stale "Result truncated" - // banner from a prior query doesn't linger while a narrowed query is in - // flight. See the matching gate in DBTimeChart. - const didOverflow = - !isPlaceholderData && data?.isComplete - ? (data?.didOverflow ?? false) - : false; + // Whether the query exceeded the row cap. See resolveDidOverflow for the + // completion + freshness gating (shared with DBTimeChart). + const didOverflow = resolveDidOverflow({ + isPlaceholderData, + isComplete: data?.isComplete, + didOverflow: data?.didOverflow, + }); const toolbarItems = useMemo(() => { const allToolbarItems: React.ReactNode[] = []; @@ -152,9 +147,7 @@ export function useCategoricalChart({ responseFormatError, didOverflow, maxResultRows, - // Row count and category (series) count of the (capped) result, surfaced so - // the overflow banner can report concrete sizes. For a categorical chart the - // number of slices/bars is the series count. + // Row + series (slice/bar) counts of the capped result, for the banner. rows: data?.rows, series: chartData.length, }; diff --git a/packages/app/src/components/charts/ChartContainer.tsx b/packages/app/src/components/charts/ChartContainer.tsx index c9ccd5ba67..f90dc8169f 100644 --- a/packages/app/src/components/charts/ChartContainer.tsx +++ b/packages/app/src/components/charts/ChartContainer.tsx @@ -9,11 +9,7 @@ interface ChartContainerProps { toolbarItems?: React.ReactNode[]; children: React.ReactNode; disableReactiveContainer?: boolean; - /** - * Optional content rendered directly below the header row and above the chart - * body — e.g. a per-tile warning banner. Takes its natural height; the chart - * body flexes to fill the remaining space. - */ + /** Content between the header and chart body — e.g. a per-tile warning banner. */ belowHeader?: React.ReactNode; } @@ -201,11 +197,9 @@ function ChartContainer({ style={{ position: 'relative', width: '100%', - // Flex-fill the remaining height (after the header and any - // belowHeader banner) instead of a rigid 100%, so a banner - // squishes the chart rather than clipping its bottom. minHeight:0 - // lets the absolutely-positioned inner chart shrink below its - // intrinsic size. + // Flex-fill remaining height (vs rigid 100%) so a belowHeader + // banner squishes the chart instead of clipping it; minHeight:0 + // lets the absolute inner chart shrink. flex: 1, minHeight: 0, }} diff --git a/packages/app/src/components/charts/HiddenSeriesIndicator.tsx b/packages/app/src/components/charts/HiddenSeriesIndicator.tsx index a9d3861591..c2630d9c2c 100644 --- a/packages/app/src/components/charts/HiddenSeriesIndicator.tsx +++ b/packages/app/src/components/charts/HiddenSeriesIndicator.tsx @@ -4,17 +4,11 @@ import { IconAlertTriangle } from '@tabler/icons-react'; interface HiddenSeriesIndicatorProps { hiddenSeriesCount: number; renderedSeriesCount: number; - /** - * Total rows returned by the query — shown alongside the series counts so the - * user can see both dimensions (rows vs series). Omit if unknown. - */ + /** Total rows returned, shown alongside the series counts. Omit if unknown. */ rowCount?: number; /** - * True when the underlying query hit the server-side row cap (see - * ResultOverflowBanner). When set, this notice must NOT claim "all series were - * loaded" — the result is a capped subset, so the series counts describe only - * what came back, not the full cardinality. Prevents the two banners from - * making contradictory completeness claims on the same tile. + * True when the query hit the row cap (see ResultOverflowBanner). Then the + * copy drops the "all series were loaded" claim, since the result is a subset. */ resultWasCapped?: boolean; /** Render every series, bypassing the cap. Omit to keep the notice passive. */ @@ -22,15 +16,9 @@ interface HiddenSeriesIndicatorProps { } /** - * Client-side SERIES cap notice. The loaded result draws only the top-N series - * (by peak value) to keep the page responsive — this surfaces how many were - * hidden and, when `onLoadAll` is provided, lets the user draw all of them - * anyway. Normally the RECOVERABLE case: all fetched data is present, it's just - * not all drawn. Contrast ResultOverflowBanner, which reports that the query may - * not have fetched everything (row cap). When `resultWasCapped` is true both - * conditions hold at once, so the copy drops the "all series were loaded" claim - * to avoid contradicting the overflow banner. Copy deliberately uses "drawn" - * here vs the row-cap banner's "missing data" so the two stay distinct. + * Notice that the chart drew only the top-N series (by peak value) to stay + * readable; `onLoadAll` lets the user draw all of them. Contrast + * ResultOverflowBanner (row cap — data the query may not have fetched). */ export default function HiddenSeriesIndicator({ hiddenSeriesCount, @@ -44,14 +32,11 @@ export default function HiddenSeriesIndicator({ } const total = renderedSeriesCount + hiddenSeriesCount; - // Row count is secondary detail (series is the primary unit for this notice). const rowsDetail = typeof rowCount === 'number' ? ` (from ${rowCount.toLocaleString()} rows)` : ''; - // When the result was capped, the loaded data is already a subset — don't - // claim "all series were loaded" (that's the overflow banner's job to - // qualify). Otherwise state completeness so the recoverable case is clear. + // When capped, don't claim "all series were loaded" (the result is a subset). const completenessClause = resultWasCapped ? `Of the ${total.toLocaleString()} series in the loaded (capped) result, ` + `only the ${renderedSeriesCount.toLocaleString()} largest (by peak value) ` + @@ -67,7 +52,11 @@ export default function HiddenSeriesIndicator({ : 'Increase the series limit, or narrow the query with a stricter GROUP BY or WHERE filter, to see more.'); const icon = ( - + ); return ( diff --git a/packages/app/src/components/charts/ResultOverflowBanner.tsx b/packages/app/src/components/charts/ResultOverflowBanner.tsx index f6e62ee523..51ef89ae70 100644 --- a/packages/app/src/components/charts/ResultOverflowBanner.tsx +++ b/packages/app/src/components/charts/ResultOverflowBanner.tsx @@ -6,33 +6,17 @@ interface ResultOverflowBannerProps { didOverflow: boolean | undefined; /** The row cap that was applied, shown in the message. */ cap: number; - /** - * Rows actually returned by the (capped) query — shown so the user sees the - * concrete size. Because the cap is block-aligned this is ~cap, not the true - * uncapped total. Omit if unknown. - */ + /** Rows returned by the capped query (≈cap since the cap is block-aligned). */ rows?: number; - /** - * Distinct series (groups) present in the capped result — shown alongside - * rows so this banner is directly comparable to the hidden-series indicator. - * Omit if unknown. - */ + /** Distinct series (groups) in the capped result. Omit if unknown. */ series?: number; } /** - * Server-side ROW cap banner, rendered just below a chart's header (via - * ChartContainer's `belowHeader` slot) when the tile's query hit the row / - * group-by cap (see DEFAULT_MAX_TILE_RESULT_ROWS). Because detection is based on - * the returned row count alone (`rows > cap`), it cannot prove the server - * actually dropped rows — a block-aligned break or a non-aggregating query can - * legitimately return a complete result that merely exceeds the cap. So the copy - * says the chart *may be missing data* rather than asserting truncation. The fix - * is always the same: narrow the query. Contrast HiddenSeriesIndicator, which - * loads the full result and only limits how many series are *drawn* (recoverable - * via "load all"); this banner is about data the query may not have fetched at - * all. Kept to one small line so it doesn't crowd the chart; the full guidance - * lives in the tooltip. + * Row-cap banner shown below a chart's header when the tile's query hit the cap + * (see DEFAULT_MAX_TILE_RESULT_ROWS). Detection is `rows > cap`, which can't + * prove rows were dropped, so the copy says the chart *may be* missing data. + * Contrast HiddenSeriesIndicator (limits only how many series are drawn). */ export default function ResultOverflowBanner({ didOverflow, @@ -44,9 +28,7 @@ export default function ResultOverflowBanner({ return null; } - // Rows is the primary unit for this banner; prefer the concrete returned-row - // count, fall back to the cap. Both are approximate (the cap is block-aligned), - // so always prefix with "~". Series is secondary detail. + // Approximate (block-aligned cap), so prefix with "~". const rowsShown = (rows ?? cap).toLocaleString(); const seriesText = typeof series === 'number' ? ` (~${series.toLocaleString()} series)` : ''; @@ -68,15 +50,12 @@ export default function ResultOverflowBanner({ py={4} px="xs" styles={{ - // Keep the alert to a single tight line regardless of the tile body's - // fs-7 / monospace context; the chart gets the remaining height. wrapper: { alignItems: 'center' }, body: { minWidth: 0 }, message: { margin: 0 }, icon: { marginRight: 8, width: 14, height: 14 }, }} - // Sits inside react-grid-layout's drag subtree; stop propagation so a - // click on the banner doesn't start a tile drag. + // Stop propagation so a click doesn't start a react-grid-layout drag. onMouseDown={e => e.stopPropagation()} > { await userEvent.click(button); expect(onLoadAll).toHaveBeenCalledTimes(1); }); + + it('states all series were loaded when the result was NOT capped', async () => { + renderWithMantine( + , + ); + // Mantine renders the tooltip label into the DOM on hover. + await userEvent.hover(screen.getByTestId('hidden-series-indicator-icon')); + expect( + await screen.findByText(/All 1,000 series were loaded/i), + ).toBeInTheDocument(); + // Row count is surfaced as secondary detail. + expect(screen.getByText(/from 1,234 rows/i)).toBeInTheDocument(); + }); + + it('does NOT claim all series were loaded when the result WAS capped', async () => { + renderWithMantine( + , + ); + await userEvent.hover(screen.getByTestId('hidden-series-indicator-icon')); + const tip = await screen.findByText(/loaded \(capped\) result/i); + expect(tip).toBeInTheDocument(); + // The contradicting "all ... were loaded" claim must be absent. + expect(tip.textContent).not.toMatch(/series were loaded/i); + }); }); diff --git a/packages/app/src/defaults.ts b/packages/app/src/defaults.ts index a6c212fe10..1f374f953d 100644 --- a/packages/app/src/defaults.ts +++ b/packages/app/src/defaults.ts @@ -1,4 +1,8 @@ -import type { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { isRawSqlChartConfig } from '@hyperdx/common-utils/dist/guards'; +import type { + BuilderChartConfigWithDateRange, + ChartConfigWithOptDateRange, +} from '@hyperdx/common-utils/dist/types'; // Limit defaults export const DEFAULT_SEARCH_ROW_LIMIT = 200; @@ -34,31 +38,66 @@ export const MAX_EXPANDED_TOOLTIP_ROWS = 500; // (HARD_LINES_LIMIT); drawn lines remain bounded by HARD_LINES_LIMIT regardless. export const MAX_LOADABLE_TIME_CHART_SERIES = 5000; -// Soft ceiling on the number of rows a single dashboard-tile query returns. -// This is a last-line defense against a pathological high-cardinality group-by -// streaming hundreds of thousands of rows into the browser — well above the -// series/draw caps, which bound what is *rendered*, whereas this bounds what is -// *transferred* (and materialized server-side). -// -// The cap is enforced by two complementary ClickHouse settings, both asked for -// with one row of HEADROOM above the logical cap (`cap + 1`, see -// `resolveResultRowLimitSetting`) so a complete result of exactly `cap` rows -// comes back whole and is NOT flagged, while a larger result trips the cap: -// 1. `max_result_rows = cap + 1` + `result_overflow_mode = 'break'` — bounds -// the RESULT rows. Weak on its own: break only stops between result blocks, -// so a result that fits in one block (up to ~65k rows) is returned whole. -// 2. `max_rows_to_group_by = cap + 1` + `group_by_overflow_mode = 'any'` — -// bounds the aggregation CARDINALITY (unique GROUP BY keys). "any" keeps the -// first N keys and folds the rest away instead of erroring. This is the -// setting that actually protects memory for a high-cardinality group-by. -// -// IMPORTANT: both are *soft*, block-aligned caps — ClickHouse checks them only -// after each data part, so the real result can overshoot `cap + 1` by up to one -// block. They are NOT exact truncations. When a query returns more than `cap` -// rows we surface an overflow banner on the tile (see `didResultOverflow`) so -// the user knows the result was capped and the chart may be missing data. +// Soft ceiling on rows a single dashboard-tile query returns — a last-line +// guard against a high-cardinality query streaming hundreds of thousands of +// rows into the browser. Bounds what is *transferred*, unlike the series/draw +// caps which bound what is *rendered*. Enforced by the ClickHouse settings in +// resolveResultRowLimitSettings; overflow is surfaced via didResultOverflow. export const DEFAULT_MAX_TILE_RESULT_ROWS = 5000; +export interface ResultRowLimitSettings { + /** `clickhouse_settings` to merge into the query request. */ + settings: Record; + /** True when the cardinality/memory guard (`max_rows_to_group_by`) applied. */ + cardinalityCapApplied: boolean; +} + +// Matches an outer `LIMIT` at the very end of a statement (tolerating a trailing +// `;` and single-line comment). Anchored to end-of-string, so a LIMIT inside a +// subquery/CTE is not matched. Conservative on purpose: a miss only falls back +// to the safe result-row cap. +const OUTER_LIMIT_RE = + /\blimit\s+\d+(?:\s*,\s*\d+)?(?:\s+offset\s+\d+)?\s*;?\s*(?:--[^\n]*)?\s*$/i; + +/** Whether a raw SQL string ends in an outer `LIMIT` clause. */ +export function hasOuterLimit(sql: string | undefined): boolean { + if (typeof sql !== 'string' || sql.length === 0) { + return false; + } + return OUTER_LIMIT_RE.test(sql.trimEnd()); +} + +/** + * ClickHouse settings that enforce the row cap. Always applies the + * order-preserving result-row cap (`max_result_rows` + `result_overflow_mode`). + * Adds the cardinality/memory cap (`max_rows_to_group_by`) ONLY when the query + * has no outer LIMIT — that cap truncates the GROUP BY before an outer + * `ORDER BY … LIMIT`, which would silently corrupt the top-N. Uses `cap + 1` + * headroom so a complete result of exactly `cap` rows isn't flagged. Returns + * undefined for a non-positive/absent cap. + */ +export function resolveResultRowLimitSettings( + cap: number | undefined, + { + hasOuterLimit: queryHasOuterLimit = false, + }: { hasOuterLimit?: boolean } = {}, +): ResultRowLimitSettings | undefined { + const limit = resolveResultRowLimitSetting(cap); + if (limit == null) { + return undefined; + } + const settings: Record = { + max_result_rows: String(limit), + result_overflow_mode: 'break', + }; + const cardinalityCapApplied = !queryHasOuterLimit; + if (cardinalityCapApplied) { + settings.max_rows_to_group_by = String(limit); + settings.group_by_overflow_mode = 'break'; + } + return { settings, cardinalityCapApplied }; +} + /** * Translate a logical row cap into the `max_result_rows` value to send to * ClickHouse. We request `cap + 1` so a result of exactly `cap` rows comes back @@ -76,25 +115,12 @@ export function resolveResultRowLimitSetting( } /** - * Decide whether a tile query exceeded the `max_result_rows` / - * `result_overflow_mode = 'break'` cap. - * - * The query is run with one row of headroom (`max_result_rows = cap + 1`; see - * `resolveResultRowLimitSetting`), so a complete result of ≤ cap rows returns - * as-is and does NOT flag; only a result larger than `cap` trips the break and - * comes back with more than `cap` rows (≥ cap + 1, possibly more since break is - * block-aligned). Hence the sole reliable signal is `rows > cap`. - * - * We deliberately do NOT use `rows_before_limit_at_least`: ClickHouse only - * populates it when the query itself has a LIMIT stage, and it then reports the - * row count BEFORE that user LIMIT — not before our result-size break. A raw - * SQL tile whose query ends in `... LIMIT 50` over a large aggregation returns - * exactly 50 rows (nothing truncated by us) yet reports - * `rows_before_limit_at_least = `, which would wrongly - * trip the banner. `rows > cap` cannot false-positive that way because the tile - * genuinely received ≤ cap rows. - * - * A non-positive/absent cap disables detection. + * Whether a tile query exceeded the row cap. Since the query runs with `cap + 1` + * headroom, `rows > cap` is the reliable signal (a complete result of ≤ cap rows + * comes back whole and is not flagged). We deliberately ignore + * `rows_before_limit_at_least`: it reports the count before the query's own + * LIMIT, so a tile ending in `... LIMIT 50` over a big aggregation would falsely + * trip. A non-positive/absent cap disables detection. */ export function didResultOverflow({ rows, @@ -112,6 +138,40 @@ export function didResultOverflow({ return false; } +/** + * The row cap a dashboard tile should apply, or undefined for none. Only raw SQL + * tiles are capped — builder tiles already bound cardinality via the series + * limit, and builder tables page rows through useOffsetPaginatedQuery. + */ +export function resolveTileMaxResultRows( + config: ChartConfigWithOptDateRange | undefined | null, +): number | undefined { + return config && isRawSqlChartConfig(config) + ? DEFAULT_MAX_TILE_RESULT_ROWS + : undefined; +} + +/** + * Whether a chart's result overflowed the row cap, gated so the banner is + * stable and never stale (shared by DBTimeChart and CategoricalChart): + * `isComplete` avoids flapping mid-stream, `!isPlaceholderData` avoids a stale + * banner lingering while a narrowed query is in flight. + */ +export function resolveDidOverflow({ + isPlaceholderData, + isComplete, + didOverflow, +}: { + isPlaceholderData: boolean | undefined; + isComplete: boolean | undefined; + didOverflow: boolean | undefined; +}): boolean { + if (isPlaceholderData || !isComplete) { + return false; + } + return didOverflow ?? false; +} + /** * Resolve the effective client-side render cap from a tile's `seriesLimit` * (see SharedChartSettingsSchema): null/undefined → the default cap, exactly 0 → diff --git a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx index 31b8c338d0..40e7f5cc2c 100644 --- a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx +++ b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx @@ -457,9 +457,10 @@ describe('useChartConfig', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)); await waitFor(() => expect(result.current.isFetching).toBe(false)); - // Both the result-row cap and the group-by cardinality cap are sent, each - // with one row of headroom (cap + 1), so the aggregation is genuinely - // bounded (not just the block-aligned result-row break). + // A builder config has no outer LIMIT of its own, so both the result-row + // cap and the group-by cardinality cap are sent, each with one row of + // headroom (cap + 1). The group-by mode is 'break' (a deterministic stop), + // NOT 'any' (which folds late keys and would corrupt an ordered top-N). expect(mockClickhouseClient.queryChartConfig).toHaveBeenCalledWith({ config, metadata: expect.any(Object), @@ -469,13 +470,90 @@ describe('useChartConfig', () => { max_result_rows: '2', result_overflow_mode: 'break', max_rows_to_group_by: '2', - group_by_overflow_mode: 'any', + group_by_overflow_mode: 'break', }, }, }); expect(result.current.data?.didOverflow).toBe(true); }); + it('omits the group-by cardinality cap for raw SQL with an outer LIMIT (avoids corrupting its top-N)', async () => { + // Raw SQL whose own `ORDER BY … LIMIT N` would run AFTER a group-by cap, + // so applying the cardinality cap could silently fold away keys before the + // top-N is computed. Only the order-preserving result-row cap is applied. + const config = createMockChartConfig({ + dateRange: undefined, + granularity: undefined, + configType: 'sql', + sqlTemplate: + 'SELECT k, count() c FROM t GROUP BY k ORDER BY c DESC LIMIT 50', + }); + + const mockResponse = createMockQueryResponse([ + { 'count()': '71', SeverityText: 'info' }, + ]); + mockClickhouseClient.queryChartConfig.mockResolvedValue(mockResponse); + + const { result } = renderHook( + () => useQueriedChartConfig(config, { maxResultRows: 1 }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + expect(mockClickhouseClient.queryChartConfig).toHaveBeenCalledWith({ + config, + metadata: expect.any(Object), + opts: { + abort_signal: expect.any(AbortSignal), + clickhouse_settings: { + readonly: '2', + max_result_rows: '2', + result_overflow_mode: 'break', + // NO max_rows_to_group_by / group_by_overflow_mode here. + }, + }, + }); + }); + + it('applies the group-by cardinality cap for raw SQL with no outer LIMIT', async () => { + const config = createMockChartConfig({ + dateRange: undefined, + granularity: undefined, + configType: 'sql', + sqlTemplate: 'SELECT k, count() c FROM t GROUP BY k', + }); + + const mockResponse = createMockQueryResponse([ + { 'count()': '71', SeverityText: 'info' }, + ]); + mockClickhouseClient.queryChartConfig.mockResolvedValue(mockResponse); + + const { result } = renderHook( + () => useQueriedChartConfig(config, { maxResultRows: 1 }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + expect(mockClickhouseClient.queryChartConfig).toHaveBeenCalledWith({ + config, + metadata: expect.any(Object), + opts: { + abort_signal: expect.any(AbortSignal), + clickhouse_settings: { + readonly: '2', + max_result_rows: '2', + result_overflow_mode: 'break', + max_rows_to_group_by: '2', + group_by_overflow_mode: 'break', + }, + }, + }); + }); + it('does not cap or flag overflow when maxResultRows is not set', async () => { const config = createMockChartConfig({ dateRange: undefined, diff --git a/packages/app/src/hooks/useChartConfig.tsx b/packages/app/src/hooks/useChartConfig.tsx index e8740ca406..3c6de01785 100644 --- a/packages/app/src/hooks/useChartConfig.tsx +++ b/packages/app/src/hooks/useChartConfig.tsx @@ -37,7 +37,11 @@ import { prometheusApi } from '@/api'; import { toStartOfInterval } from '@/ChartUtils'; import { useClickhouseClient } from '@/clickhouse'; import { IS_MTVIEWS_ENABLED } from '@/config'; -import { didResultOverflow, resolveResultRowLimitSetting } from '@/defaults'; +import { + didResultOverflow, + hasOuterLimit, + resolveResultRowLimitSettings, +} from '@/defaults'; import { buildMTViewSelectQuery } from '@/hdxMTViews'; import { useMetadataWithSettings } from '@/hooks/useMetadata'; import { useSource } from '@/source'; @@ -56,12 +60,9 @@ interface AdditionalUseQueriedChartConfigOptions { enableQueryChunking?: boolean; enableParallelQueries?: boolean; /** - * When set to a positive number, caps the number of rows the query is allowed - * to return via the ClickHouse `max_result_rows` + `result_overflow_mode = - * 'break'` settings, and reports whether the cap was hit via - * `data.didOverflow`. Used by dashboard tiles as a last-line guard against a - * high-cardinality query streaming an unbounded number of rows into the - * browser. Omit (or pass 0/undefined) to leave the query uncapped. + * When positive, caps the rows the query may return (see + * resolveResultRowLimitSettings) and reports whether the cap was hit via + * `data.didOverflow`. Omit / 0 to leave uncapped. */ maxResultRows?: number; } @@ -210,30 +211,16 @@ async function* fetchDataInChunks({ ? { readonly: '2' } : {}; - // Cap the number of rows any single (possibly chunked) query returns. We ask - // ClickHouse for one row of headroom above the logical cap - // (`max_result_rows = cap + 1`, via resolveResultRowLimitSetting) with - // `result_overflow_mode = 'break'`, so a complete result of exactly `cap` - // rows comes back whole (no false overflow warning) while a larger result - // trips the break and returns cap + 1 rows. The caller detects the overflow - // via the returned `rows` count (see didResultOverflow) and warns the user. - // - // `max_result_rows` alone is a WEAK guard: `result_overflow_mode = 'break'` - // only stops between result blocks, so a result that fits in one block (up to - // ~65k rows) is returned whole and nothing is actually capped. We therefore - // ALSO cap the aggregation cardinality with `max_rows_to_group_by` + - // `group_by_overflow_mode = 'any'`, which bounds the number of unique GROUP BY - // keys the query materializes ("any" keeps the first N keys and folds the - // rest away instead of erroring). This is the setting that actually protects - // memory for a high-cardinality group-by. Both are block-aligned, so the real - // result can overshoot the cap by up to one block — detection stays tolerant - // (rows > cap). Only positive finite caps are applied. - const resultRowLimit = resolveResultRowLimitSetting(maxResultRows); - if (resultRowLimit != null) { - clickHouseSettings.max_result_rows = String(resultRowLimit); - clickHouseSettings.result_overflow_mode = 'break'; - clickHouseSettings.max_rows_to_group_by = String(resultRowLimit); - clickHouseSettings.group_by_overflow_mode = 'any'; + // Cap the rows any single (possibly chunked) query returns. The chosen + // settings depend on whether the raw SQL has its own outer LIMIT (which would + // be corrupted by the group-by cardinality cap) — see + // resolveResultRowLimitSettings. Builder configs have no sqlTemplate. + const rawSql = isRawSqlChartConfig(config) ? config.sqlTemplate : undefined; + const resultRowLimitSettings = resolveResultRowLimitSettings(maxResultRows, { + hasOuterLimit: hasOuterLimit(rawSql), + }); + if (resultRowLimitSettings != null) { + Object.assign(clickHouseSettings, resultRowLimitSettings.settings); } if (enableParallelQueries) { @@ -478,10 +465,9 @@ export function useQueriedChartConfig( maxResultRows: options?.maxResultRows, }); - // A chunked range applies the cap per chunk, so the accumulated total can - // legitimately exceed the cap without any single chunk overflowing. Track - // whether ANY individual chunk hit the cap (the definitive per-query - // signal) and OR it with the whole-result check below. + // The cap applies per chunk, so `didOverflow` is the OR of the per-chunk + // `rows > cap` check (raw SQL never chunks, so its one chunk is the whole + // result). const cap = options?.maxResultRows; let anyChunkOverflowed = false; From 4e4785e5d266ada779827113f360306996c4da90 Mon Sep 17 00:00:00 2001 From: Brandon Pereira Date: Mon, 10 Aug 2026 14:57:46 -0600 Subject: [PATCH 3/5] fix(dashboard): detect outer LIMIT with trailing SETTINGS/FORMAT/WITH TIES (greptile P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasOuterLimit's end-anchored regex missed an outer LIMIT followed by a valid trailing clause (`LIMIT 50 SETTINGS ...`, `LIMIT 50 FORMAT JSON`, `LIMIT 50 WITH TIES`). Those queries would fall through to the group-by cardinality cap, which truncates aggregation before the outer ORDER BY/LIMIT selects the top-N — the exact silent-corruption case the LIMIT check exists to prevent. Strip trailing SETTINGS/FORMAT/comment/semicolon clauses before the LIMIT test and accept an optional WITH TIES. Adds tests for each shape. --- packages/app/src/__tests__/defaults.test.ts | 29 +++++++++++++++++++++ packages/app/src/defaults.ts | 20 +++++++++----- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/packages/app/src/__tests__/defaults.test.ts b/packages/app/src/__tests__/defaults.test.ts index 09b47738ac..54154dacec 100644 --- a/packages/app/src/__tests__/defaults.test.ts +++ b/packages/app/src/__tests__/defaults.test.ts @@ -135,6 +135,26 @@ describe('hasOuterLimit', () => { expect(hasOuterLimit('SELECT k, count() FROM t GROUP BY k')).toBe(false); }); + it('detects an outer LIMIT followed by trailing SETTINGS / FORMAT / WITH TIES', () => { + // ClickHouse allows these after LIMIT; missing them would wrongly enable the + // group-by cap and corrupt the top-N (greptile P1). + expect( + hasOuterLimit('SELECT * FROM t LIMIT 50 SETTINGS max_threads=4'), + ).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 SETTINGS a=1, b=2;')).toBe( + true, + ); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 FORMAT JSON')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t ORDER BY c LIMIT 50 WITH TIES')).toBe( + true, + ); + expect( + hasOuterLimit( + 'SELECT * FROM t LIMIT 50 WITH TIES SETTINGS max_threads=4', + ), + ).toBe(true); + }); + it('does not treat a LIMIT inside a subquery as an outer LIMIT', () => { // Anchored to end-of-string: an inner LIMIT followed by more query is not // matched, so the cardinality cap can still apply. @@ -143,6 +163,15 @@ describe('hasOuterLimit', () => { ).toBe(false); }); + it('does not treat a trailing SETTINGS / FORMAT without a LIMIT as an outer LIMIT', () => { + expect( + hasOuterLimit( + 'SELECT k, count() FROM t GROUP BY k SETTINGS max_threads=4', + ), + ).toBe(false); + expect(hasOuterLimit('SELECT * FROM t FORMAT JSON')).toBe(false); + }); + it('returns false for empty / non-string input', () => { expect(hasOuterLimit('')).toBe(false); expect(hasOuterLimit(undefined)).toBe(false); diff --git a/packages/app/src/defaults.ts b/packages/app/src/defaults.ts index 1f374f953d..6fd0d1b824 100644 --- a/packages/app/src/defaults.ts +++ b/packages/app/src/defaults.ts @@ -52,19 +52,27 @@ export interface ResultRowLimitSettings { cardinalityCapApplied: boolean; } -// Matches an outer `LIMIT` at the very end of a statement (tolerating a trailing -// `;` and single-line comment). Anchored to end-of-string, so a LIMIT inside a -// subquery/CTE is not matched. Conservative on purpose: a miss only falls back -// to the safe result-row cap. +// Clauses ClickHouse allows AFTER an outer `LIMIT` (SETTINGS/FORMAT), plus a +// trailing `;` and single-line comment. Stripped from the end before the LIMIT +// test so a query like `... LIMIT 50 SETTINGS max_threads=4` still counts as +// having an outer LIMIT (missing it would wrongly enable the group-by cap and +// corrupt the top-N). +const TRAILING_CLAUSE_RE = + /(?:\s+settings\s+.+|\s+format\s+\w+|\s*;|\s*--[^\n]*)+\s*$/i; +// Outer `LIMIT` at the end: `LIMIT n [,m] [OFFSET n] [WITH TIES]`. Anchored to +// end-of-string (after trailing clauses are stripped), so a LIMIT inside a +// subquery/CTE is not matched. Conservative: a miss only falls back to the safe +// result-row cap. const OUTER_LIMIT_RE = - /\blimit\s+\d+(?:\s*,\s*\d+)?(?:\s+offset\s+\d+)?\s*;?\s*(?:--[^\n]*)?\s*$/i; + /\blimit\s+\d+(?:\s*,\s*\d+)?(?:\s+offset\s+\d+)?(?:\s+with\s+ties)?\s*$/i; /** Whether a raw SQL string ends in an outer `LIMIT` clause. */ export function hasOuterLimit(sql: string | undefined): boolean { if (typeof sql !== 'string' || sql.length === 0) { return false; } - return OUTER_LIMIT_RE.test(sql.trimEnd()); + const withoutTrailing = sql.trimEnd().replace(TRAILING_CLAUSE_RE, ''); + return OUTER_LIMIT_RE.test(withoutTrailing); } /** From 0d59719482e2d5e217b8e7866dd6d4607b2d1583 Mon Sep 17 00:00:00 2001 From: Brandon Pereira Date: Mon, 10 Aug 2026 15:22:21 -0600 Subject: [PATCH 4/5] fix(dashboard): harden outer-LIMIT detection; add parallel-path + regex tests Follow-up review fixes (greptile P1 + deep-review P2s): - hasOuterLimit now strips trailing block comments (/* ... */) before the LIMIT test and recognizes the `LIMIT n BY col` form. Both were false negatives that would wrongly enable the group-by cardinality cap and silently corrupt a raw-SQL top-N. The LIMIT BY matcher is end-anchored and excludes ')' so an inner-subquery LIMIT BY isn't matched. - Tests: block-comment / LIMIT BY / subquery-LIMIT-BY cases for hasOuterLimit, and a parallel-query (enableParallelQueries) assertion that each chunk query carries the row-cap clickhouse_settings. - P3: rename the singular helper resolveResultRowLimitSetting -> resolveMaxResultRowsValue to disambiguate from the plural settings builder. Deferred (deep-review P2): GROUP BY ... ORDER BY with no LIMIT still applies the cardinality cap by design (keeps the memory guard); the banner wording for that case is left as a follow-up. --- packages/app/src/__tests__/defaults.test.ts | 49 ++++++++++++++----- packages/app/src/defaults.ts | 37 +++++++++----- .../hooks/__tests__/useChartConfig.test.tsx | 38 ++++++++++++++ 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/packages/app/src/__tests__/defaults.test.ts b/packages/app/src/__tests__/defaults.test.ts index 54154dacec..f6cd05c2d4 100644 --- a/packages/app/src/__tests__/defaults.test.ts +++ b/packages/app/src/__tests__/defaults.test.ts @@ -5,30 +5,28 @@ import { didResultOverflow, hasOuterLimit, resolveDidOverflow, - resolveResultRowLimitSetting, + resolveMaxResultRowsValue, resolveResultRowLimitSettings, resolveTileMaxResultRows, } from '@/defaults'; -describe('resolveResultRowLimitSetting', () => { +describe('resolveMaxResultRowsValue', () => { it('requests one row of headroom above the cap', () => { // cap + 1 lets a complete result of exactly `cap` rows come back whole, // while anything larger trips the break. - expect(resolveResultRowLimitSetting(5000)).toBe(5001); - expect(resolveResultRowLimitSetting(1)).toBe(2); + expect(resolveMaxResultRowsValue(5000)).toBe(5001); + expect(resolveMaxResultRowsValue(1)).toBe(2); }); it('floors non-integer caps before adding headroom', () => { - expect(resolveResultRowLimitSetting(99.9)).toBe(100); + expect(resolveMaxResultRowsValue(99.9)).toBe(100); }); it('returns undefined for a non-positive / absent cap (no limit applied)', () => { - expect(resolveResultRowLimitSetting(undefined)).toBeUndefined(); - expect(resolveResultRowLimitSetting(0)).toBeUndefined(); - expect(resolveResultRowLimitSetting(-10)).toBeUndefined(); - expect( - resolveResultRowLimitSetting(Number.POSITIVE_INFINITY), - ).toBeUndefined(); + expect(resolveMaxResultRowsValue(undefined)).toBeUndefined(); + expect(resolveMaxResultRowsValue(0)).toBeUndefined(); + expect(resolveMaxResultRowsValue(-10)).toBeUndefined(); + expect(resolveMaxResultRowsValue(Number.POSITIVE_INFINITY)).toBeUndefined(); }); }); @@ -155,12 +153,41 @@ describe('hasOuterLimit', () => { ).toBe(true); }); + it('detects an outer LIMIT followed by a trailing block comment', () => { + // /* ... */ after the LIMIT must not defeat detection (greptile P1). + expect( + hasOuterLimit( + 'SELECT k, count() c FROM t GROUP BY k ORDER BY c DESC LIMIT 50 /* dashboard note */', + ), + ).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50 /* note */;')).toBe(true); + expect(hasOuterLimit('SELECT * FROM t LIMIT 50\n/* multi\nline */')).toBe( + true, + ); + }); + + it('detects an outer LIMIT … BY clause', () => { + expect(hasOuterLimit('SELECT * FROM t ORDER BY c LIMIT 10 BY host')).toBe( + true, + ); + expect(hasOuterLimit('SELECT * FROM t LIMIT 2 BY host, region;')).toBe( + true, + ); + // `LIMIT … BY … LIMIT m` is caught via the trailing regular LIMIT. + expect(hasOuterLimit('SELECT * FROM t LIMIT 1 BY host LIMIT 50')).toBe( + true, + ); + }); + it('does not treat a LIMIT inside a subquery as an outer LIMIT', () => { // Anchored to end-of-string: an inner LIMIT followed by more query is not // matched, so the cardinality cap can still apply. expect( hasOuterLimit('SELECT * FROM (SELECT * FROM t LIMIT 10) GROUP BY k'), ).toBe(false); + expect( + hasOuterLimit('SELECT * FROM (SELECT * FROM t LIMIT 5 BY h) GROUP BY k'), + ).toBe(false); }); it('does not treat a trailing SETTINGS / FORMAT without a LIMIT as an outer LIMIT', () => { diff --git a/packages/app/src/defaults.ts b/packages/app/src/defaults.ts index 6fd0d1b824..ab6c880de7 100644 --- a/packages/app/src/defaults.ts +++ b/packages/app/src/defaults.ts @@ -53,26 +53,37 @@ export interface ResultRowLimitSettings { } // Clauses ClickHouse allows AFTER an outer `LIMIT` (SETTINGS/FORMAT), plus a -// trailing `;` and single-line comment. Stripped from the end before the LIMIT -// test so a query like `... LIMIT 50 SETTINGS max_threads=4` still counts as -// having an outer LIMIT (missing it would wrongly enable the group-by cap and -// corrupt the top-N). +// trailing `;`, single-line comment, and block comment. Stripped from the end +// (repeatedly) before the LIMIT test so a query like +// `... LIMIT 50 SETTINGS max_threads=4` or `... LIMIT 50 /* note */` still +// counts as having an outer LIMIT (missing it would wrongly enable the group-by +// cap and corrupt the top-N). const TRAILING_CLAUSE_RE = - /(?:\s+settings\s+.+|\s+format\s+\w+|\s*;|\s*--[^\n]*)+\s*$/i; -// Outer `LIMIT` at the end: `LIMIT n [,m] [OFFSET n] [WITH TIES]`. Anchored to -// end-of-string (after trailing clauses are stripped), so a LIMIT inside a -// subquery/CTE is not matched. Conservative: a miss only falls back to the safe -// result-row cap. + /(?:\s+settings\s+.+|\s+format\s+\w+|\s*;|\s*--[^\n]*|\s*\/\*[\s\S]*?\*\/)+\s*$/i; +// Outer `LIMIT` at the end (after trailing clauses are stripped): +// `LIMIT n [,m] [OFFSET n] [WITH TIES]` or `LIMIT n BY col…`. Anchored to +// end-of-string, so a LIMIT inside a subquery/CTE is not matched. Conservative: +// a miss only falls back to the safe result-row cap. const OUTER_LIMIT_RE = /\blimit\s+\d+(?:\s*,\s*\d+)?(?:\s+offset\s+\d+)?(?:\s+with\s+ties)?\s*$/i; +// `LIMIT n BY ` (ClickHouse LIMIT BY) at the end — still an outer LIMIT +// that runs after ORDER BY. Anchored to end-of-string; the column list may not +// contain a `)` (which would indicate a subquery's LIMIT BY, not an outer one). +// A `LIMIT … BY … LIMIT m` shape is already caught by OUTER_LIMIT_RE via its +// trailing `LIMIT m`. +const OUTER_LIMIT_BY_RE = + /\blimit\s+\d+(?:\s*,\s*\d+)?\s+by\s+[\w.,\s'"[\]]+$/i; -/** Whether a raw SQL string ends in an outer `LIMIT` clause. */ +/** Whether a raw SQL string ends in an outer `LIMIT` (incl. `LIMIT … BY`). */ export function hasOuterLimit(sql: string | undefined): boolean { if (typeof sql !== 'string' || sql.length === 0) { return false; } const withoutTrailing = sql.trimEnd().replace(TRAILING_CLAUSE_RE, ''); - return OUTER_LIMIT_RE.test(withoutTrailing); + return ( + OUTER_LIMIT_RE.test(withoutTrailing) || + OUTER_LIMIT_BY_RE.test(withoutTrailing) + ); } /** @@ -90,7 +101,7 @@ export function resolveResultRowLimitSettings( hasOuterLimit: queryHasOuterLimit = false, }: { hasOuterLimit?: boolean } = {}, ): ResultRowLimitSettings | undefined { - const limit = resolveResultRowLimitSetting(cap); + const limit = resolveMaxResultRowsValue(cap); if (limit == null) { return undefined; } @@ -113,7 +124,7 @@ export function resolveResultRowLimitSettings( * and returns at least `cap + 1` rows. Returns undefined for a non-positive / * absent cap (no limit applied). */ -export function resolveResultRowLimitSetting( +export function resolveMaxResultRowsValue( cap: number | undefined, ): number | undefined { if (cap == null || !Number.isFinite(cap) || cap <= 0) { diff --git a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx index 40e7f5cc2c..38326a0893 100644 --- a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx +++ b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx @@ -1571,6 +1571,44 @@ describe('useChartConfig', () => { expect(result.current.isPending).toBe(false); }); + it('passes the row-cap clickhouse_settings on the parallel path', async () => { + const { config, mockResponse1, mockResponse2, mockResponse3 } = + setupParallelQueries(); + + mockClickhouseClient.queryChartConfig + .mockResolvedValueOnce(mockResponse1) + .mockResolvedValueOnce(mockResponse2) + .mockResolvedValueOnce(mockResponse3); + + const { result } = renderHook( + () => + useQueriedChartConfig(config, { + enableQueryChunking: true, + enableParallelQueries: true, + maxResultRows: 5000, + }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true), { + timeout: 1000, + }); + await waitFor(() => expect(result.current.isFetching).toBe(false), { + timeout: 1000, + }); + + // Every parallel chunk query carries the cap settings (builder config has + // no outer LIMIT, so the cardinality cap applies too), each with cap + 1. + for (const call of mockClickhouseClient.queryChartConfig.mock.calls) { + expect(call[0].opts?.clickhouse_settings).toEqual({ + max_result_rows: '5001', + result_overflow_mode: 'break', + max_rows_to_group_by: '5001', + group_by_overflow_mode: 'break', + }); + } + }); + it('streams parallel query results in order', async () => { const { config, mockResponse1, mockResponse2, mockResponse3 } = setupParallelQueries(); From 9c5f2a709e7f2a7c078a19c3249f614c0d34f890 Mon Sep 17 00:00:00 2001 From: Brandon Pereira Date: Mon, 10 Aug 2026 15:29:11 -0600 Subject: [PATCH 5/5] fix(dashboard): consume multiline trailing SETTINGS in outer-LIMIT detection (greptile P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TRAILING_CLAUSE_RE used `settings\s+.+`, and `.` doesn't match newlines, so a `LIMIT 50 SETTINGS\n max_threads=4` clause wrapped onto multiple lines wasn't fully stripped — hasOuterLimit returned false and the group-by cardinality cap was wrongly enabled, silently corrupting the top-N. Use `[\s\S]+` so a multiline SETTINGS clause is consumed. Adds a multiline-SETTINGS test. --- packages/app/src/__tests__/defaults.test.ts | 6 ++++++ packages/app/src/defaults.ts | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/app/src/__tests__/defaults.test.ts b/packages/app/src/__tests__/defaults.test.ts index f6cd05c2d4..b9dabe4639 100644 --- a/packages/app/src/__tests__/defaults.test.ts +++ b/packages/app/src/__tests__/defaults.test.ts @@ -151,6 +151,12 @@ describe('hasOuterLimit', () => { 'SELECT * FROM t LIMIT 50 WITH TIES SETTINGS max_threads=4', ), ).toBe(true); + // SETTINGS wrapped onto multiple lines must still be consumed (greptile P1). + expect( + hasOuterLimit( + 'SELECT k, count() c FROM t GROUP BY k ORDER BY c DESC LIMIT 50 SETTINGS\n max_threads=4,\n max_memory_usage=1000', + ), + ).toBe(true); }); it('detects an outer LIMIT followed by a trailing block comment', () => { diff --git a/packages/app/src/defaults.ts b/packages/app/src/defaults.ts index ab6c880de7..219a60b729 100644 --- a/packages/app/src/defaults.ts +++ b/packages/app/src/defaults.ts @@ -57,9 +57,10 @@ export interface ResultRowLimitSettings { // (repeatedly) before the LIMIT test so a query like // `... LIMIT 50 SETTINGS max_threads=4` or `... LIMIT 50 /* note */` still // counts as having an outer LIMIT (missing it would wrongly enable the group-by -// cap and corrupt the top-N). +// cap and corrupt the top-N). SETTINGS uses `[\s\S]+` (not `.+`) so a clause +// wrapped onto multiple lines is still fully consumed. const TRAILING_CLAUSE_RE = - /(?:\s+settings\s+.+|\s+format\s+\w+|\s*;|\s*--[^\n]*|\s*\/\*[\s\S]*?\*\/)+\s*$/i; + /(?:\s+settings\s+[\s\S]+|\s+format\s+\w+|\s*;|\s*--[^\n]*|\s*\/\*[\s\S]*?\*\/)+\s*$/i; // Outer `LIMIT` at the end (after trailing clauses are stripped): // `LIMIT n [,m] [OFFSET n] [WITH TIES]` or `LIMIT n BY col…`. Anchored to // end-of-string, so a LIMIT inside a subquery/CTE is not matched. Conservative: