From e8021532a29743ed757539b0276d243aebf5eb81 Mon Sep 17 00:00:00 2001 From: Jared Scott Date: Fri, 4 Sep 2026 14:03:58 +0800 Subject: [PATCH 1/2] fix(histogram): restore temporal execution and rendering DRC-4216 Co-Authored-By: Claude Signed-off-by: Jared Scott --- js/packages/storybook/.storybook/preview.tsx | 1 - js/packages/storybook/package.json | 1 - js/packages/ui/package.json | 1 + js/packages/ui/src/api/index.ts | 1 + js/packages/ui/src/api/profile.ts | 9 +- .../ui/src/components/data/HistogramChart.tsx | 8 +- .../HistogramChart.geometry.test.tsx | 50 +++++++++- .../data/__tests__/HistogramChart.test.tsx | 30 +++--- .../histogram/HistogramResultView.tsx | 51 ++++++++-- .../__tests__/HistogramResultView.test.tsx | 63 ++++++++++++- js/pnpm-lock.yaml | 6 +- .../HistogramDiffResultView.test.tsx | 52 ++++++++++ recce/tasks/histogram.py | 37 ++++---- tests/tasks/test_histogram.py | 94 ++++++++++++++++++- 14 files changed, 349 insertions(+), 55 deletions(-) diff --git a/js/packages/storybook/.storybook/preview.tsx b/js/packages/storybook/.storybook/preview.tsx index ab1cebe69..4a30b5735 100644 --- a/js/packages/storybook/.storybook/preview.tsx +++ b/js/packages/storybook/.storybook/preview.tsx @@ -2,7 +2,6 @@ import { theme } from "@datarecce/ui/theme"; import CssBaseline from "@mui/material/CssBaseline"; import { ThemeProvider as MuiThemeProvider } from "@mui/material/styles"; import type { Preview } from "@storybook/react-vite"; -import "chartjs-adapter-date-fns"; import { useEffect } from "react"; // Import styles from @datarecce/ui diff --git a/js/packages/storybook/package.json b/js/packages/storybook/package.json index 85eda1f55..53ae562bd 100644 --- a/js/packages/storybook/package.json +++ b/js/packages/storybook/package.json @@ -27,7 +27,6 @@ "@vitejs/plugin-react": "^6.0.5", "@vitest/browser-playwright": "^4.1.10", "@vitest/coverage-v8": "^4.1.10", - "chartjs-adapter-date-fns": "^3.0.0", "msw": "^2.15.0", "playwright": "^1.62.1", "storybook": "^10.5.7", diff --git a/js/packages/ui/package.json b/js/packages/ui/package.json index 81eddffb6..0482c6de1 100644 --- a/js/packages/ui/package.json +++ b/js/packages/ui/package.json @@ -157,6 +157,7 @@ "ag-grid-community": "^36.0.1", "ag-grid-react": "^36.0.1", "chart.js": "^4.5.0", + "chartjs-adapter-date-fns": "^3.0.0", "common-tags": "^1.8.0", "date-fns": "^4.1.0", "file-saver": "^2.0.5", diff --git a/js/packages/ui/src/api/index.ts b/js/packages/ui/src/api/index.ts index 2bd790095..8aa727144 100644 --- a/js/packages/ui/src/api/index.ts +++ b/js/packages/ui/src/api/index.ts @@ -117,6 +117,7 @@ export type { HistogramDiffParams, HistogramDiffResult, HistogramResult, + HistogramWireValue, ProfileDiffParams, ProfileDiffResult, ProfileDiffViewOptions, diff --git a/js/packages/ui/src/api/profile.ts b/js/packages/ui/src/api/profile.ts index cee6cb6b6..58bb30f52 100644 --- a/js/packages/ui/src/api/profile.ts +++ b/js/packages/ui/src/api/profile.ts @@ -93,6 +93,7 @@ export interface HistogramDiffParams { model: string; column_name: string; column_type: string; + num_bins?: number; } export interface HistogramResult { @@ -100,11 +101,13 @@ export interface HistogramResult { total: number; } +export type HistogramWireValue = number | string; + export interface HistogramDiffResult { base: HistogramResult; current: HistogramResult; - min: number; - max: number; - bin_edges: number[]; + min: HistogramWireValue | null; + max: HistogramWireValue | null; + bin_edges: HistogramWireValue[]; labels?: string[]; } diff --git a/js/packages/ui/src/components/data/HistogramChart.tsx b/js/packages/ui/src/components/data/HistogramChart.tsx index 1b8f0ff72..7c22cabd2 100644 --- a/js/packages/ui/src/components/data/HistogramChart.tsx +++ b/js/packages/ui/src/components/data/HistogramChart.tsx @@ -1,5 +1,6 @@ "use client"; +import "chartjs-adapter-date-fns"; import { BarElement, CategoryScale, @@ -100,6 +101,7 @@ function formatBinRange( } const MAX_EDGE_TICK_LABELS = 8; +type HistogramChartValue = number | { x: number; y: number }; /** * HistogramChart Component @@ -181,7 +183,7 @@ function HistogramChartComponent({ ); // Build chart data - const chartData = useMemo>(() => { + const chartData = useMemo>(() => { const labels = binEdges .slice(0, -1) .map((_, i) => @@ -195,7 +197,7 @@ function HistogramChartComponent({ ) => { const counts = data.counts ?? []; const chartValues = isDatetime - ? counts.map((v, i) => [binEdges[i], v] as [number, number]) + ? counts.map((v, i) => ({ x: binEdges[i], y: v })) : isNumeric ? counts.map((v, i) => ({ x: (binEdges[i] + binEdges[i + 1]) / 2, @@ -205,7 +207,7 @@ function HistogramChartComponent({ return { label, - data: chartValues as number[], + data: chartValues, backgroundColor: colors.chartFill, borderColor: colors.border, hoverBackgroundColor: colors.chartFill, diff --git a/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx b/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx index 3c13c82cc..07c463461 100644 --- a/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx +++ b/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx @@ -12,7 +12,7 @@ import { vi } from "vitest"; import { HistogramChart } from "../HistogramChart"; interface CapturedChartProps { - data: ChartData<"bar">; + data: ChartData<"bar", (number | { x: number; y: number })[]>; options: ChartOptions<"bar">; plugins?: Plugin<"bar">[]; } @@ -82,7 +82,53 @@ function createCanvas() { }; } -describe("HistogramChart numeric geometry", () => { +describe("HistogramChart real Chart.js configuration", () => { + it("constructs the emitted timeseries configuration with real Chart.js", () => { + const binEdges = [ + Date.UTC(2026, 0, 1), + Date.UTC(2026, 0, 2), + Date.UTC(2026, 0, 3), + ]; + render( + , + ); + + const { data, options } = getLastChartProps(); + const chart = new ChartJS(createCanvas() as never, { + type: "bar", + data, + options: { + ...options, + animation: false, + plugins: { + ...options.plugins, + legend: { display: false }, + title: { ...options.plugins?.title, display: false }, + }, + responsive: false, + }, + platform: BasicPlatform, + }); + + try { + expect(chart.scales.x.type).toBe("timeseries"); + expect(chart.scales.x.min).toBe(binEdges[0]); + expect(chart.scales.x.max).toBe(binEdges[2]); + expect(chart.getDatasetMeta(0).data).toHaveLength(2); + expect(chart.scales.x.getLabelForValue(binEdges[0])).toContain("2026"); + } finally { + chart.destroy(); + } + }); + it.each([ { binEdges: [0, 20], counts: [5] }, { binEdges: [0, 20, 40, 60, 80, 100], counts: [1, 2, 3, 4, 5] }, diff --git a/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx b/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx index f40cc3750..721df0566 100644 --- a/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx +++ b/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx @@ -101,6 +101,8 @@ vi.mock("chart.js", () => ({ Tooltip: {}, })); +vi.mock("chartjs-adapter-date-fns", () => ({})); + function getLastChartProps(): MockChartProps { const props = chartSpy.mock.lastCall?.[0] as MockChartProps | undefined; if (!props) throw new Error("Chart was not rendered"); @@ -377,7 +379,7 @@ describe("HistogramChart", () => { expect(options?.scales?.x?.type).toBe("category"); }); - it("keeps datetime labels, tuples, scale, and hidden axis unchanged", () => { + it("emits datetime points as objects on the timeseries scale", () => { render( { "80 - 100", ]); expect(data.datasets[0].data).toEqual([ - [0, 15], - [20, 25], - [40, 35], - [60, 45], - [80, 55], + { x: 0, y: 15 }, + { x: 20, y: 25 }, + { x: 40, y: 35 }, + { x: 60, y: 45 }, + { x: 80, y: 55 }, ]); expect(data.datasets[1].data).toEqual([ - [0, 10], - [20, 20], - [40, 30], - [60, 40], - [80, 50], + { x: 0, y: 10 }, + { x: 20, y: 20 }, + { x: 40, y: 30 }, + { x: 60, y: 40 }, + { x: 80, y: 50 }, ]); expect(options?.scales?.x).toMatchObject({ display: false, @@ -685,7 +687,7 @@ describe("HistogramChart", () => { }); describe("data transformation", () => { - it("creates [timestamp, count] tuples for datetime type", () => { + it("creates timestamp/count objects for datetime type", () => { const datetimeBinEdges = [ 1704067200000, 1704672000000, 1705276800000, 1705881600000, 1706486400000, 1706745600000, @@ -704,10 +706,8 @@ describe("HistogramChart", () => { const chart = getByTestId("mock-chart"); const data = JSON.parse(chart.getAttribute("data-data") || "{}"); - // For datetime, data should be arrays of [x, y] tuples const currentData = data.datasets[0].data; - expect(Array.isArray(currentData[0])).toBe(true); - expect(currentData[0]).toHaveLength(2); + expect(currentData[0]).toEqual({ x: 1704067200000, y: 15 }); }); it("uses midpoint coordinate objects for numeric type", () => { diff --git a/js/packages/ui/src/components/histogram/HistogramResultView.tsx b/js/packages/ui/src/components/histogram/HistogramResultView.tsx index 38f585e38..b7f29bd27 100644 --- a/js/packages/ui/src/components/histogram/HistogramResultView.tsx +++ b/js/packages/ui/src/components/histogram/HistogramResultView.tsx @@ -17,12 +17,14 @@ import type { ForwardRefExoticComponent, RefAttributes } from "react"; import { type HistogramDiffParams, type HistogramDiffResult, + type HistogramWireValue, isHistogramDiffRun, type Run, } from "../../api"; import { HistogramChart, type HistogramDataType } from "../../primitives"; import { createResultView } from "../result/createResultView"; import type { CreatedResultViewProps, ResultViewData } from "../result/types"; +import { classifyType } from "../ui/DataTypeIcon/classifyType"; // ============================================================================ // Type Definitions @@ -53,6 +55,26 @@ function isHistogramDiffRunGuard(run: unknown): run is HistogramDiffRun { return isHistogramDiffRun(run as Run); } +function isTemporalHistogramType(columnType: string): boolean { + const category = classifyType(columnType); + if (category === "date" || category === "datetime") { + return true; + } + + const normalizedType = columnType.trim().toUpperCase().replace(/\s+/g, " "); + return normalizedType === "YEAR" || normalizedType === "INTERVAL"; +} + +function toEpochMilliseconds(value: HistogramWireValue): number { + return typeof value === "number" ? value : Date.parse(value); +} + +function toOptionalEpochMilliseconds( + value: HistogramWireValue | null | undefined, +): number | undefined { + return value == null ? undefined : toEpochMilliseconds(value); +} + // ============================================================================ // Factory-Created Component // ============================================================================ @@ -100,12 +122,23 @@ export const HistogramDiffResultView = createResultView< // Map column_type to HistogramDataType const columnType = (run.params?.column_type ?? "numeric") as string; - const dataType: HistogramDataType = - columnType === "datetime" - ? "datetime" - : columnType === "string" - ? "string" - : "numeric"; + const dataType: HistogramDataType = isTemporalHistogramType(columnType) + ? "datetime" + : columnType === "string" + ? "string" + : "numeric"; + const chartMin = + dataType === "datetime" + ? toOptionalEpochMilliseconds(min) + : (min as number | undefined); + const chartMax = + dataType === "datetime" + ? toOptionalEpochMilliseconds(max) + : (max as number | undefined); + const chartBinEdges = + dataType === "datetime" + ? binEdges.map(toEpochMilliseconds) + : (binEdges as number[]); return { content: ( @@ -117,10 +150,10 @@ export const HistogramDiffResultView = createResultView< dataType={dataType} baseData={{ counts: base.counts }} currentData={{ counts: current.counts }} - min={min} - max={max} + min={chartMin} + max={chartMax} samples={base.total} - binEdges={binEdges} + binEdges={chartBinEdges} theme={isDark ? "dark" : "light"} /> diff --git a/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx b/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx index 03fcd9c6c..42c1251d6 100644 --- a/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx +++ b/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx @@ -6,7 +6,7 @@ import type { HistogramDiffRun } from "../HistogramResultView"; import { HistogramDiffResultView } from "../HistogramResultView"; interface MockChartProps { - data: { labels?: number[]; datasets: { data: number[] }[] }; + data: { labels?: unknown[]; datasets: { data: unknown[] }[] }; options?: { scales?: { x?: { @@ -45,6 +45,8 @@ vi.mock("chart.js", () => ({ Tooltip: {}, })); +vi.mock("chartjs-adapter-date-fns", () => ({})); + vi.mock("../../../hooks", () => ({ useIsDark: () => false, })); @@ -105,4 +107,63 @@ describe("HistogramDiffResultView", () => { }); expect(tickCallback?.(100, 5, [])).toBe("100"); }); + + it.each([ + "DATE", + "date", + "DATETIME", + "DATETIME(6)", + "TIMESTAMP", + "TIMESTAMP(6)", + "YEAR", + "DATETIME2", + "SMALLDATETIME", + "DATETIMEOFFSET", + "INTERVAL", + "TIMESTAMPTZ", + "TIMESTAMPTZ(6)", + "TIMESTAMP WITH TIME ZONE", + "timestamp without time zone", + " timestamp ( 6 ) with time zone ", + "TIMESTAMP WITH LOCAL TIME ZONE", + "TIMESTAMP_LTZ", + "TIMESTAMP_NTZ", + "TIMESTAMP_TZ", + ])("normalizes %s ISO wire values at the chart boundary", (columnType) => { + const run: HistogramDiffRun = { + type: "histogram_diff", + run_id: "temporal-histogram-run", + run_at: "2026-08-25T00:00:00Z", + status: "Finished", + params: { + model: "orders", + column_name: "created_at", + column_type: columnType, + }, + result: { + base: { counts: [2, 3], total: 5 }, + current: { counts: [5, 7], total: 12 }, + min: "2026-01-01T00:00:00.000Z", + max: "2026-01-03T00:00:00.000Z", + bin_edges: ["2026-01-01", "2026-01-02", "2026-01-03"], + }, + }; + + render( + + + , + ); + + const { data, options } = getLastChartProps(); + expect(options?.scales?.x).toMatchObject({ + min: Date.UTC(2026, 0, 1), + max: Date.UTC(2026, 0, 3), + type: "timeseries", + }); + expect(data.datasets[0].data).toEqual([ + { x: Date.UTC(2026, 0, 1), y: 5 }, + { x: Date.UTC(2026, 0, 2), y: 7 }, + ]); + }); }); diff --git a/js/pnpm-lock.yaml b/js/pnpm-lock.yaml index d460bf6ff..8761921b7 100644 --- a/js/pnpm-lock.yaml +++ b/js/pnpm-lock.yaml @@ -354,9 +354,6 @@ importers: '@vitest/coverage-v8': specifier: ^4.1.10 version: 4.1.10(@vitest/browser@4.1.10)(vitest@4.1.10) - chartjs-adapter-date-fns: - specifier: ^3.0.0 - version: 3.0.0(chart.js@4.5.1)(date-fns@4.4.0) msw: specifier: ^2.15.0 version: 2.15.0(@types/node@26.2.0)(@typescript/typescript6@6.0.2) @@ -441,6 +438,9 @@ importers: chart.js: specifier: ^4.5.0 version: 4.5.1 + chartjs-adapter-date-fns: + specifier: ^3.0.0 + version: 3.0.0(chart.js@4.5.1)(date-fns@4.4.0) common-tags: specifier: ^1.8.0 version: 1.8.2 diff --git a/js/src/components/histogram/HistogramDiffResultView.test.tsx b/js/src/components/histogram/HistogramDiffResultView.test.tsx index 88862f708..8cb7b9890 100644 --- a/js/src/components/histogram/HistogramDiffResultView.test.tsx +++ b/js/src/components/histogram/HistogramDiffResultView.test.tsx @@ -472,6 +472,58 @@ describe("HistogramDiffResultView", () => { ); }); + it.each([ + { checkId: undefined, columnType: "DATE", source: "direct" }, + { checkId: "saved-date", columnType: "DATE", source: "saved" }, + { checkId: undefined, columnType: "TIMESTAMP", source: "direct" }, + { + checkId: "saved-timestamp", + columnType: "TIMESTAMP", + source: "saved", + }, + { checkId: undefined, columnType: "TIMESTAMPTZ", source: "direct" }, + { + checkId: "saved-timestamptz", + columnType: "TIMESTAMPTZ", + source: "saved", + }, + ])( + "normalizes $source $columnType histogram wire dates for the OSS chart", + ({ checkId, columnType }) => { + const run = createHistogramDiffRun(); + run.check_id = checkId; + run.params = { + model: "orders", + column_name: "created_at", + column_type: columnType, + }; + // Deliberately mirror saved-state/API JSON rather than constructing Date + // objects, so both OSS loading paths exercise the public wire contract. + run.result = { + base: { counts: [2, 3], total: 5 }, + current: { counts: [5, 7], total: 12 }, + min: "2026-01-01T00:00:00Z", + max: "2026-01-03T00:00:00Z", + bin_edges: ["2026-01-01", "2026-01-02", "2026-01-03"], + }; + + renderWithProviders(); + + expect(mockHistogramChart).toHaveBeenCalledWith( + expect.objectContaining({ + dataType: "datetime", + min: Date.UTC(2026, 0, 1), + max: Date.UTC(2026, 0, 3), + binEdges: [ + Date.UTC(2026, 0, 1), + Date.UTC(2026, 0, 2), + Date.UTC(2026, 0, 3), + ], + }), + ); + }, + ); + it("maps string column_type to string dataType", () => { const run = createHistogramDiffRun(); // Preserve required params and override column_type diff --git a/recce/tasks/histogram.py b/recce/tasks/histogram.py index 21fa527c5..81b282491 100644 --- a/recce/tasks/histogram.py +++ b/recce/tasks/histogram.py @@ -379,7 +379,7 @@ def query_datetime_histogram(task, node, column, min_value, max_value): interval_years = math.ceil((dmax.year - dmin.year) / 50) interval = relativedelta(years=+interval_years) num_buckets = math.ceil((dmax.year - dmin.year) / interval.years) - bin_edges = [dmin + relativedelta(year=i) for i in range(num_buckets + 1)] + bin_edges = [dmin + interval * i for i in range(num_buckets + 1)] sql = f""" SELECT {{{{ date_trunc("year", "{column}") }}}} as year, @@ -418,7 +418,7 @@ def query_datetime_histogram(task, node, column, min_value, max_value): else: dmax = date(3000, 1, 1) num_buckets = (dmax - dmin).days - bin_edges = [dmin + relativedelta(day=i) for i in range(num_buckets + 1)] + bin_edges = [dmin + interval * i for i in range(num_buckets + 1)] sql = f""" SELECT {{{{ date_trunc("day", "{column}") }}}} as day, @@ -444,21 +444,26 @@ def query_datetime_histogram(task, node, column, min_value, max_value): finally: task.check_cancel() - base_counts = [0] * num_buckets print(_type) - for d, v in base.rows: - i = bin_edges.index(d.date()) if isinstance(d, datetime) else bin_edges.index(d) - base_counts[i] = v - curr_counts = [0] * num_buckets - for d, v in curr.rows: - i = bin_edges.index(d.date()) if isinstance(d, datetime) else bin_edges.index(d) - curr_counts[i] = v - base_result = { - "counts": base_counts, - } - curr_result = { - "counts": curr_counts, - } + + def build_result(query_result): + if query_result is None: + return {} + + counts = [0] * num_buckets + for value, count in query_result.rows: + edge_value = value.date() if isinstance(value, datetime) else value + if _type == "yearly": + index = (edge_value.year - dmin.year) // interval.years + else: + index = bin_edges.index(edge_value) + if index < 0 or index >= num_buckets: + raise ValueError(f"Histogram date {edge_value} is outside the computed edge domain") + counts[index] += count + return {"counts": counts} + + base_result = build_result(base) + curr_result = build_result(curr) return base_result, curr_result, bin_edges diff --git a/tests/tasks/test_histogram.py b/tests/tasks/test_histogram.py index 75b8fb0ce..ba00ff744 100644 --- a/tests/tasks/test_histogram.py +++ b/tests/tasks/test_histogram.py @@ -1,11 +1,12 @@ import json -from datetime import date +from datetime import date, datetime from decimal import Decimal from pathlib import Path from types import SimpleNamespace from unittest.mock import MagicMock import pytest +from fastapi.encoders import jsonable_encoder import recce.tasks.histogram as histogram from recce.tasks.histogram import ( @@ -123,6 +124,97 @@ def check_cancel(self): return None +class DatetimeHistogramQueryTask: + def __init__(self, base_rows=(), current_rows=(), *, fail_base=False, fail_current=False): + self.base_rows = HistogramRows(base_rows) + self.current_rows = HistogramRows(current_rows) + self.fail_base = fail_base + self.fail_current = fail_current + + def execute_sql(self, _sql, *, base): + if (base and self.fail_base) or (not base and self.fail_current): + raise RuntimeError("warehouse query failed") + return self.base_rows if base else self.current_rows + + def check_cancel(self): + return None + + +def test_daily_datetime_histogram_edges_cross_months_monotonically(): + """Catch absolute ``relativedelta(day=...)`` fields resetting edges within a month.""" + task = DatetimeHistogramQueryTask( + base_rows=[(datetime(2026, 1, 30), 2), (date(2026, 1, 31), 3)], + current_rows=[(date(2026, 2, 1), 5), (datetime(2026, 2, 2), 7)], + ) + + base, current, bin_edges = histogram.query_datetime_histogram( + task, + "orders", + "created_at", + date(2026, 1, 30), + date(2026, 2, 2), + ) + + assert bin_edges == [ + date(2026, 1, 30), + date(2026, 1, 31), + date(2026, 2, 1), + date(2026, 2, 2), + date(2026, 2, 3), + ] + assert all(left < right for left, right in zip(bin_edges, bin_edges[1:])) + serialized_edges = jsonable_encoder(bin_edges) + assert serialized_edges == ["2026-01-30", "2026-01-31", "2026-02-01", "2026-02-02", "2026-02-03"] + assert serialized_edges == sorted(serialized_edges) + assert base == {"counts": [2, 3, 0, 0]} + assert current == {"counts": [0, 0, 5, 7]} + + +def test_yearly_datetime_histogram_uses_additive_interval_spacing(): + """Catch absolute years and one-year SQL rows escaping a multi-year bucket.""" + task = DatetimeHistogramQueryTask( + base_rows=[(date(1875, 1, 1), 2), (date(1878, 1, 1), 3), (date(1879, 1, 1), 5)], + current_rows=[(date(2026, 1, 1), 7)], + ) + + base, current, bin_edges = histogram.query_datetime_histogram( + task, + "orders", + "created_at", + date(1875, 6, 1), + date(2026, 8, 1), + ) + + assert bin_edges[0] == date(1875, 1, 1) + assert bin_edges[-1] == date(2027, 1, 1) + assert all(right.year - left.year == 4 for left, right in zip(bin_edges, bin_edges[1:])) + serialized_edges = jsonable_encoder(bin_edges) + assert serialized_edges == sorted(serialized_edges) + assert base["counts"][:2] == [5, 5] + assert sum(base["counts"]) == 10 + assert current["counts"][-1] == 7 + + +def test_datetime_histogram_preserves_the_successful_side_when_one_query_fails(): + """Catch a handled warehouse failure becoming a secondary ``None.rows`` exception.""" + task = DatetimeHistogramQueryTask( + current_rows=[(date(2026, 1, 1), 4)], + fail_base=True, + ) + + base, current, bin_edges = histogram.query_datetime_histogram( + task, + "orders", + "created_at", + date(2026, 1, 1), + date(2026, 1, 1), + ) + + assert bin_edges == [date(2026, 1, 1), date(2026, 1, 2)] + assert base == {} + assert current == {"counts": [4]} + + class DecimalExtremaHistogramTask(HistogramDiffTask): def __init__(self, minimum, maximum, column_type="NUMERIC"): super().__init__({"model": "numbers", "column_name": "amount", "column_type": column_type, "num_bins": 1}) From 4ca722653ee1faa381af9802d419028d56705a63 Mon Sep 17 00:00:00 2001 From: Jared Scott Date: Fri, 4 Sep 2026 14:56:35 +0800 Subject: [PATCH 2/2] fix(histogram): harden temporal boundaries DRC-4216 Co-Authored-By: Claude Signed-off-by: Jared Scott --- js/packages/ui/src/api/profile.ts | 2 +- .../ui/src/components/data/HistogramChart.tsx | 49 ++++++-- .../HistogramChart.geometry.test.tsx | 26 ++++- .../data/__tests__/HistogramChart.test.tsx | 76 +++++++++--- .../histogram/HistogramDiffForm.tsx | 8 +- .../histogram/HistogramResultView.tsx | 63 +++++----- .../__tests__/HistogramResultView.test.tsx | 110 +++++++++++++++++- .../src/components/histogram/histogramType.ts | 29 +++++ .../src/components/histogram/histogramWire.ts | 36 ++++++ .../HistogramDiffResultView.test.tsx | 86 ++++++-------- recce/tasks/histogram.py | 55 ++++++--- tests/fixtures/histogram_type_policy.json | 16 +++ tests/tasks/test_histogram.py | 91 ++++++++++++++- 13 files changed, 498 insertions(+), 149 deletions(-) create mode 100644 js/packages/ui/src/components/histogram/histogramType.ts create mode 100644 js/packages/ui/src/components/histogram/histogramWire.ts diff --git a/js/packages/ui/src/api/profile.ts b/js/packages/ui/src/api/profile.ts index 58bb30f52..cb9c99910 100644 --- a/js/packages/ui/src/api/profile.ts +++ b/js/packages/ui/src/api/profile.ts @@ -109,5 +109,5 @@ export interface HistogramDiffResult { min: HistogramWireValue | null; max: HistogramWireValue | null; bin_edges: HistogramWireValue[]; - labels?: string[]; + labels?: string[] | null; } diff --git a/js/packages/ui/src/components/data/HistogramChart.tsx b/js/packages/ui/src/components/data/HistogramChart.tsx index 7c22cabd2..d669654ef 100644 --- a/js/packages/ui/src/components/data/HistogramChart.tsx +++ b/js/packages/ui/src/components/data/HistogramChart.tsx @@ -100,6 +100,14 @@ function formatBinRange( return `${formatEdge(start)} - ${formatEdge(end)}`; } +function formatUtcHistogramEdge(value: number): string { + if (!Number.isFinite(value)) return String(value); + const iso = new Date(value).toISOString(); + return iso.endsWith("T00:00:00.000Z") + ? iso.slice(0, 10) + : iso.replace(".000Z", "Z"); +} + const MAX_EDGE_TICK_LABELS = 8; type HistogramChartValue = number | { x: number; y: number }; @@ -144,8 +152,6 @@ function HistogramChartComponent({ title, dataType = "numeric", samples = 0, - min = 0, - max = 0, binEdges, baseData, currentData, @@ -161,6 +167,8 @@ function HistogramChartComponent({ const comparisonColors = semanticColors.comparison; const isDatetime = dataType === "datetime"; const isNumeric = dataType === "numeric"; + const baseCounts = baseData.counts ?? []; + const currentCounts = currentData.counts ?? []; const formatNumericEdge = useMemo( () => createHistogramEdgeFormatter(binEdges), [binEdges], @@ -187,7 +195,15 @@ function HistogramChartComponent({ const labels = binEdges .slice(0, -1) .map((_, i) => - formatBinRange(binEdges, i, isNumeric ? formatNumericEdge : undefined), + formatBinRange( + binEdges, + i, + isNumeric + ? formatNumericEdge + : isDatetime + ? formatUtcHistogramEdge + : undefined, + ), ); const buildDataset = ( @@ -197,7 +213,10 @@ function HistogramChartComponent({ ) => { const counts = data.counts ?? []; const chartValues = isDatetime - ? counts.map((v, i) => ({ x: binEdges[i], y: v })) + ? counts.map((v, i) => ({ + x: (binEdges[i] + binEdges[i + 1]) / 2, + y: v, + })) : isNumeric ? counts.map((v, i) => ({ x: (binEdges[i] + binEdges[i + 1]) / 2, @@ -242,7 +261,7 @@ function HistogramChartComponent({ // Build chart options const chartOptions = useMemo>(() => { - const maxCount = Math.max(...currentData.counts, ...baseData.counts); + const maxCount = Math.max(0, ...currentCounts, ...baseCounts); const edgeTickInterval = Math.max( 1, Math.ceil((binEdges.length - 1) / (MAX_EDGE_TICK_LABELS - 1)), @@ -309,13 +328,16 @@ function HistogramChartComponent({ const range = formatBinRange( binEdges, dataIndex, - isNumeric ? formatNumericEdge : undefined, + isNumeric + ? formatNumericEdge + : isDatetime + ? formatUtcHistogramEdge + : undefined, ); return `${dataTypeLabel}\n${range}`; }, label({ datasetIndex, dataIndex, dataset }) { - const counts = - datasetIndex === 0 ? currentData.counts : baseData.counts; + const counts = datasetIndex === 0 ? currentCounts : baseCounts; const count = counts[dataIndex]; const percent = samples > 0 ? formatIntervalMinMax(count / samples) : ""; @@ -329,8 +351,8 @@ function HistogramChartComponent({ ? { display: !hideAxis, type: "timeseries", - min, - max, + min: firstEdge, + max: terminalEdge, adapters: { date: {} }, time: { minUnit: "day" }, grid: { display: false }, @@ -338,6 +360,9 @@ function HistogramChartComponent({ minRotation: 30, maxRotation: 30, maxTicksLimit: 8, + callback(value) { + return formatUtcHistogramEdge(value as number); + }, color: themeColors.textColor, }, } @@ -396,11 +421,11 @@ function HistogramChartComponent({ isDatetime, isNumeric, samples, - min, - max, binEdges, baseData, currentData, + baseCounts, + currentCounts, hideAxis, animate, themeColors, diff --git a/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx b/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx index 07c463461..94de7f47a 100644 --- a/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx +++ b/js/packages/ui/src/components/data/__tests__/HistogramChart.geometry.test.tsx @@ -26,6 +26,7 @@ interface RenderedBar { y?: { active(): boolean }; }; hidden: boolean; + skip?: boolean; width: number; x: number; y: number; @@ -83,18 +84,18 @@ function createCanvas() { } describe("HistogramChart real Chart.js configuration", () => { - it("constructs the emitted timeseries configuration with real Chart.js", () => { + it("keeps every temporal interval inside edge-derived bounds with real Chart.js", () => { const binEdges = [ Date.UTC(2026, 0, 1), Date.UTC(2026, 0, 2), - Date.UTC(2026, 0, 3), + Date.UTC(2026, 0, 4), ]; render( { expect(chart.scales.x.type).toBe("timeseries"); expect(chart.scales.x.min).toBe(binEdges[0]); expect(chart.scales.x.max).toBe(binEdges[2]); - expect(chart.getDatasetMeta(0).data).toHaveLength(2); - expect(chart.scales.x.getLabelForValue(binEdges[0])).toContain("2026"); + expect(data.datasets[0].data).toEqual([ + { x: Date.UTC(2026, 0, 1, 12), y: 5 }, + { x: Date.UTC(2026, 0, 3), y: 7 }, + ]); + const bars = chart.getDatasetMeta(0).data as unknown as RenderedBar[]; + expect(bars).toHaveLength(2); + for (const bar of bars) { + expect(bar.skip).not.toBe(true); + expect(bar.x - bar.width / 2).toBeGreaterThanOrEqual( + chart.chartArea.left, + ); + expect(bar.x + bar.width / 2).toBeLessThanOrEqual( + chart.chartArea.right, + ); + } } finally { chart.destroy(); } diff --git a/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx b/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx index 721df0566..ab5ca4450 100644 --- a/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx +++ b/js/packages/ui/src/components/data/__tests__/HistogramChart.test.tsx @@ -60,6 +60,7 @@ interface MockChartProps { ) => string | undefined; }; }; + y?: { max?: number }; }; }; plugins?: Plugin<"bar">[]; @@ -379,7 +380,7 @@ describe("HistogramChart", () => { expect(options?.scales?.x?.type).toBe("category"); }); - it("emits datetime points as objects on the timeseries scale", () => { + it("centers datetime points within edge-derived timeseries bounds", () => { render( { const { data, options } = getLastChartProps(); expect(data.labels).toEqual([ - "0 - 20", - "20 - 40", - "40 - 60", - "60 - 80", - "80 - 100", + "1970-01-01 - 1970-01-01T00:00:00.020Z", + "1970-01-01T00:00:00.020Z - 1970-01-01T00:00:00.040Z", + "1970-01-01T00:00:00.040Z - 1970-01-01T00:00:00.060Z", + "1970-01-01T00:00:00.060Z - 1970-01-01T00:00:00.080Z", + "1970-01-01T00:00:00.080Z - 1970-01-01T00:00:00.100Z", ]); expect(data.datasets[0].data).toEqual([ - { x: 0, y: 15 }, - { x: 20, y: 25 }, - { x: 40, y: 35 }, - { x: 60, y: 45 }, - { x: 80, y: 55 }, + { x: 10, y: 15 }, + { x: 30, y: 25 }, + { x: 50, y: 35 }, + { x: 70, y: 45 }, + { x: 90, y: 55 }, ]); expect(data.datasets[1].data).toEqual([ - { x: 0, y: 10 }, - { x: 20, y: 20 }, - { x: 40, y: 30 }, - { x: 60, y: 40 }, - { x: 80, y: 50 }, + { x: 10, y: 10 }, + { x: 30, y: 20 }, + { x: 50, y: 30 }, + { x: 70, y: 40 }, + { x: 90, y: 50 }, ]); expect(options?.scales?.x).toMatchObject({ display: false, + max: 100, + min: 0, type: "timeseries", }); }); + it("formats datetime axis and tooltip ranges as UTC", () => { + const binEdges = [ + Date.UTC(2026, 0, 1), + Date.UTC(2026, 0, 2), + Date.UTC(2026, 0, 3, 12, 30), + ]; + render( + , + ); + + const { options } = getLastChartProps(); + const tickCallback = options?.scales?.x?.ticks?.callback; + const titleCallback = options?.plugins?.tooltip?.callbacks?.title; + expect(tickCallback?.(binEdges[0], 0, [])).toBe("2026-01-01"); + expect(tickCallback?.(binEdges[2], 2, [])).toBe("2026-01-03T12:30:00Z"); + expect(titleCallback?.([{ dataIndex: 1 }])).toBe( + "Date Range\n2026-01-02 - 2026-01-03T12:30:00Z", + ); + }); + + it("keeps the y-axis maximum finite for empty datasets", () => { + render( + , + ); + + expect(getLastChartProps().options?.scales?.y?.max).toBe(0); + }); + it("creates datasets with correct labels", () => { const { getByTestId } = render(); @@ -707,7 +749,7 @@ describe("HistogramChart", () => { const data = JSON.parse(chart.getAttribute("data-data") || "{}"); const currentData = data.datasets[0].data; - expect(currentData[0]).toEqual({ x: 1704067200000, y: 15 }); + expect(currentData[0]).toEqual({ x: 1704369600000, y: 15 }); }); it("uses midpoint coordinate objects for numeric type", () => { diff --git a/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx b/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx index 9c604a8f8..be626894e 100644 --- a/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx +++ b/js/packages/ui/src/components/histogram/HistogramDiffForm.tsx @@ -16,6 +16,7 @@ import type { HistogramDiffParams } from "../../api"; import { useModelColumns } from "../../hooks"; import type { RunFormProps } from "../run"; import { classifyType } from "../ui/DataTypeIcon/classifyType"; +import { isDatetimeHistogramType } from "./histogramType"; // ============================================================================ // Type Utilities @@ -65,12 +66,7 @@ function isBooleanDataType(columnType: string) { function isDateTimeType(columnType: string) { const category = classifyType(columnType); - if (["date", "datetime", "time"].includes(category)) { - return true; - } - - const normalizedType = columnType.trim().toUpperCase(); - return normalizedType === "YEAR" || normalizedType === "INTERVAL"; + return category === "time" || isDatetimeHistogramType(columnType); } // ============================================================================ diff --git a/js/packages/ui/src/components/histogram/HistogramResultView.tsx b/js/packages/ui/src/components/histogram/HistogramResultView.tsx index b7f29bd27..77f1cf011 100644 --- a/js/packages/ui/src/components/histogram/HistogramResultView.tsx +++ b/js/packages/ui/src/components/histogram/HistogramResultView.tsx @@ -24,7 +24,8 @@ import { import { HistogramChart, type HistogramDataType } from "../../primitives"; import { createResultView } from "../result/createResultView"; import type { CreatedResultViewProps, ResultViewData } from "../result/types"; -import { classifyType } from "../ui/DataTypeIcon/classifyType"; +import { isDatetimeHistogramType } from "./histogramType"; +import { parseHistogramTimestamp } from "./histogramWire"; // ============================================================================ // Type Definitions @@ -55,24 +56,10 @@ function isHistogramDiffRunGuard(run: unknown): run is HistogramDiffRun { return isHistogramDiffRun(run as Run); } -function isTemporalHistogramType(columnType: string): boolean { - const category = classifyType(columnType); - if (category === "date" || category === "datetime") { - return true; - } - - const normalizedType = columnType.trim().toUpperCase().replace(/\s+/g, " "); - return normalizedType === "YEAR" || normalizedType === "INTERVAL"; -} - -function toEpochMilliseconds(value: HistogramWireValue): number { - return typeof value === "number" ? value : Date.parse(value); -} - function toOptionalEpochMilliseconds( value: HistogramWireValue | null | undefined, ): number | undefined { - return value == null ? undefined : toEpochMilliseconds(value); + return value == null ? undefined : parseHistogramTimestamp(value); } // ============================================================================ @@ -122,23 +109,35 @@ export const HistogramDiffResultView = createResultView< // Map column_type to HistogramDataType const columnType = (run.params?.column_type ?? "numeric") as string; - const dataType: HistogramDataType = isTemporalHistogramType(columnType) + const dataType: HistogramDataType = isDatetimeHistogramType(columnType) ? "datetime" : columnType === "string" ? "string" : "numeric"; - const chartMin = - dataType === "datetime" - ? toOptionalEpochMilliseconds(min) - : (min as number | undefined); - const chartMax = - dataType === "datetime" - ? toOptionalEpochMilliseconds(max) - : (max as number | undefined); - const chartBinEdges = - dataType === "datetime" - ? binEdges.map(toEpochMilliseconds) - : (binEdges as number[]); + let chartMin: number | undefined; + let chartMax: number | undefined; + let chartBinEdges: number[]; + try { + chartMin = + dataType === "datetime" + ? toOptionalEpochMilliseconds(min) + : (min as number | undefined); + chartMax = + dataType === "datetime" + ? toOptionalEpochMilliseconds(max) + : (max as number | undefined); + chartBinEdges = + dataType === "datetime" + ? binEdges.map(parseHistogramTimestamp) + : (binEdges as number[]); + } catch { + return { + content:
Invalid histogram data
, + }; + } + + const baseCounts = Array.isArray(base.counts) ? base.counts : []; + const currentCounts = Array.isArray(current.counts) ? current.counts : []; return { content: ( @@ -148,11 +147,11 @@ export const HistogramDiffResultView = createResultView< diff --git a/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx b/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx index 42c1251d6..8acc58fca 100644 --- a/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx +++ b/js/packages/ui/src/components/histogram/__tests__/HistogramResultView.test.tsx @@ -1,5 +1,5 @@ import { ThemeProvider } from "@mui/material/styles"; -import { render } from "@testing-library/react"; +import { render, screen } from "@testing-library/react"; import { vi } from "vitest"; import { theme } from "../../../theme"; import type { HistogramDiffRun } from "../HistogramResultView"; @@ -86,6 +86,7 @@ describe("HistogramDiffResultView", () => { min: 0, max: 100, bin_edges: [0, 20, 40, 60, 80, 100], + labels: null, }, }; @@ -116,10 +117,14 @@ describe("HistogramDiffResultView", () => { "TIMESTAMP", "TIMESTAMP(6)", "YEAR", + "YEAR(4)", + " year ( 4 ) ", "DATETIME2", "SMALLDATETIME", "DATETIMEOFFSET", "INTERVAL", + "INTERVAL(6)", + " interval ( 6 ) ", "TIMESTAMPTZ", "TIMESTAMPTZ(6)", "TIMESTAMP WITH TIME ZONE", @@ -162,8 +167,107 @@ describe("HistogramDiffResultView", () => { type: "timeseries", }); expect(data.datasets[0].data).toEqual([ - { x: Date.UTC(2026, 0, 1), y: 5 }, - { x: Date.UTC(2026, 0, 2), y: 7 }, + { x: Date.UTC(2026, 0, 1, 12), y: 5 }, + { x: Date.UTC(2026, 0, 2, 12), y: 7 }, ]); }); + + it.each([ + { + name: "date-only", + min: "2026-01-01", + max: "2026-01-02", + edges: ["2026-01-01", "2026-01-02"], + expected: [Date.UTC(2026, 0, 1), Date.UTC(2026, 0, 2)], + }, + { + name: "timezone-less FastAPI datetime", + min: "2026-01-01T01:02:03.456", + max: "2026-01-01T02:02:03.456", + edges: ["2026-01-01T01:02:03.456", "2026-01-01T02:02:03.456"], + expected: [ + Date.UTC(2026, 0, 1, 1, 2, 3, 456), + Date.UTC(2026, 0, 1, 2, 2, 3, 456), + ], + }, + { + name: "offset datetime", + min: "2026-01-01T00:00:00-08:00", + max: "2026-01-01T01:00:00-08:00", + edges: ["2026-01-01T00:00:00-08:00", "2026-01-01T01:00:00-08:00"], + expected: [Date.UTC(2026, 0, 1, 8), Date.UTC(2026, 0, 1, 9)], + }, + ])( + "parses $name wire values deterministically", + ({ min, max, edges, expected }) => { + const run: HistogramDiffRun = { + type: "histogram_diff", + run_id: "wire-date-run", + run_at: "2026-08-25T00:00:00Z", + status: "Finished", + params: { + model: "orders", + column_name: "created_at", + column_type: "TIMESTAMP", + }, + result: { + base: { counts: [2], total: 2 }, + current: { counts: [5], total: 5 }, + min, + max, + bin_edges: edges, + }, + }; + + render( + + + , + ); + + const { data, options } = getLastChartProps(); + expect(options?.scales?.x).toMatchObject({ + min: expected[0], + max: expected[1], + type: "timeseries", + }); + expect(data.datasets[0].data).toEqual([ + { x: (expected[0] + expected[1]) / 2, y: 5 }, + ]); + }, + ); + + it.each(["not-an-iso-date", Number.POSITIVE_INFINITY])( + "renders a safe empty state for invalid temporal wire value %s", + (invalidValue) => { + chartSpy.mockClear(); + const run: HistogramDiffRun = { + type: "histogram_diff", + run_id: "invalid-wire-date-run", + run_at: "2026-08-25T00:00:00Z", + status: "Finished", + params: { + model: "orders", + column_name: "created_at", + column_type: "TIMESTAMP", + }, + result: { + base: { counts: [2], total: 2 }, + current: { counts: [5], total: 5 }, + min: invalidValue, + max: "2026-01-02", + bin_edges: ["2026-01-01", "2026-01-02"], + }, + }; + + render( + + + , + ); + + expect(screen.getByText("Invalid histogram data")).toBeInTheDocument(); + expect(chartSpy).not.toHaveBeenCalled(); + }, + ); }); diff --git a/js/packages/ui/src/components/histogram/histogramType.ts b/js/packages/ui/src/components/histogram/histogramType.ts new file mode 100644 index 000000000..fa09583ce --- /dev/null +++ b/js/packages/ui/src/components/histogram/histogramType.ts @@ -0,0 +1,29 @@ +const DATETIME_HISTOGRAM_TYPES = new Set([ + "DATE", + "DATETIME", + "TIMESTAMP", + "YEAR", + "DATETIME2", + "SMALLDATETIME", + "DATETIMEOFFSET", + "INTERVAL", + "TIMESTAMPTZ", + "TIMESTAMP WITH TIME ZONE", + "TIMESTAMP WITHOUT TIME ZONE", + "TIMESTAMP WITH LOCAL TIME ZONE", + "TIMESTAMP_LTZ", + "TIMESTAMP_NTZ", + "TIMESTAMP_TZ", +]); + +const TYPE_PRECISION_PATTERN = /\s*\(\s*\d+\s*\)/; + +/** Mirrors the normalized temporal classifier used by the histogram backend. */ +export function isDatetimeHistogramType(columnType: string): boolean { + const normalizedType = columnType.trim().toUpperCase().replace(/\s+/g, " "); + const typeWithoutPrecision = normalizedType.replace( + TYPE_PRECISION_PATTERN, + "", + ); + return DATETIME_HISTOGRAM_TYPES.has(typeWithoutPrecision); +} diff --git a/js/packages/ui/src/components/histogram/histogramWire.ts b/js/packages/ui/src/components/histogram/histogramWire.ts new file mode 100644 index 000000000..5df12239a --- /dev/null +++ b/js/packages/ui/src/components/histogram/histogramWire.ts @@ -0,0 +1,36 @@ +import type { HistogramWireValue } from "../../api"; + +const ISO_DATE_PATTERN = /^\d{4}-\d{2}-\d{2}$/; +const ISO_DATETIME_PATTERN = + /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:\d{2})?$/i; +const ISO_TIME_ZONE_PATTERN = /(?:Z|[+-]\d{2}:\d{2})$/i; + +/** Parse the FastAPI ISO wire format without depending on the browser timezone. */ +export function parseHistogramTimestamp(value: HistogramWireValue): number { + if (typeof value === "number") { + if (Number.isFinite(value)) return value; + throw new RangeError("Histogram timestamp must be finite"); + } + + const trimmedValue = value.trim(); + let normalizedValue: string; + if (ISO_DATE_PATTERN.test(trimmedValue)) { + normalizedValue = `${trimmedValue}T00:00:00Z`; + } else if (ISO_DATETIME_PATTERN.test(trimmedValue)) { + normalizedValue = ISO_TIME_ZONE_PATTERN.test(trimmedValue) + ? trimmedValue + : `${trimmedValue}Z`; + } else { + throw new RangeError( + "Histogram timestamp must use an ISO date or datetime", + ); + } + + const timestamp = Date.parse(normalizedValue); + if (!Number.isFinite(timestamp)) { + throw new RangeError( + "Histogram timestamp is outside the supported date range", + ); + } + return timestamp; +} diff --git a/js/src/components/histogram/HistogramDiffResultView.test.tsx b/js/src/components/histogram/HistogramDiffResultView.test.tsx index 8cb7b9890..7b7ea7f10 100644 --- a/js/src/components/histogram/HistogramDiffResultView.test.tsx +++ b/js/src/components/histogram/HistogramDiffResultView.test.tsx @@ -97,6 +97,7 @@ vi.mock("@datarecce/ui/hooks", () => ({ // ============================================================================ import { HistogramDiffResultView } from "@datarecce/ui/components/histogram"; +import { findByRunType } from "@datarecce/ui/components/run"; import { screen } from "@testing-library/react"; import React from "react"; import { @@ -472,57 +473,40 @@ describe("HistogramDiffResultView", () => { ); }); - it.each([ - { checkId: undefined, columnType: "DATE", source: "direct" }, - { checkId: "saved-date", columnType: "DATE", source: "saved" }, - { checkId: undefined, columnType: "TIMESTAMP", source: "direct" }, - { - checkId: "saved-timestamp", - columnType: "TIMESTAMP", - source: "saved", - }, - { checkId: undefined, columnType: "TIMESTAMPTZ", source: "direct" }, - { - checkId: "saved-timestamptz", - columnType: "TIMESTAMPTZ", - source: "saved", - }, - ])( - "normalizes $source $columnType histogram wire dates for the OSS chart", - ({ checkId, columnType }) => { - const run = createHistogramDiffRun(); - run.check_id = checkId; - run.params = { - model: "orders", - column_name: "created_at", - column_type: columnType, - }; - // Deliberately mirror saved-state/API JSON rather than constructing Date - // objects, so both OSS loading paths exercise the public wire contract. - run.result = { - base: { counts: [2, 3], total: 5 }, - current: { counts: [5, 7], total: 12 }, - min: "2026-01-01T00:00:00Z", - max: "2026-01-03T00:00:00Z", - bin_edges: ["2026-01-01", "2026-01-02", "2026-01-03"], - }; - - renderWithProviders(); - - expect(mockHistogramChart).toHaveBeenCalledWith( - expect.objectContaining({ - dataType: "datetime", - min: Date.UTC(2026, 0, 1), - max: Date.UTC(2026, 0, 3), - binEdges: [ - Date.UTC(2026, 0, 1), - Date.UTC(2026, 0, 2), - Date.UTC(2026, 0, 3), - ], - }), - ); - }, - ); + it("normalizes a serialized saved run through the OSS run registry", () => { + const run = createHistogramDiffRun(); + run.params = { + model: "orders", + column_name: "created_at", + column_type: " year ( 4 ) ", + }; + run.result = { + base: { counts: [2, 3], total: 5 }, + current: { counts: [5, 7], total: 12 }, + min: "2026-01-01T00:00:00", + max: "2026-01-02T16:00:00-08:00", + bin_edges: ["2026-01-01", "2026-01-02", "2026-01-03"], + }; + const savedRun = JSON.parse(JSON.stringify(run)) as typeof run; + const SavedResultView = findByRunType(savedRun.type).RunResultView; + if (!SavedResultView) + throw new Error("Histogram result view is not registered"); + + renderWithProviders(); + + expect(mockHistogramChart).toHaveBeenCalledWith( + expect.objectContaining({ + dataType: "datetime", + min: Date.UTC(2026, 0, 1), + max: Date.UTC(2026, 0, 3), + binEdges: [ + Date.UTC(2026, 0, 1), + Date.UTC(2026, 0, 2), + Date.UTC(2026, 0, 3), + ], + }), + ); + }); it("maps string column_type to string dataType", () => { const run = createHistogramDiffRun(); diff --git a/recce/tasks/histogram.py b/recce/tasks/histogram.py index 81b282491..3736def4c 100644 --- a/recce/tasks/histogram.py +++ b/recce/tasks/histogram.py @@ -1,5 +1,6 @@ import math import re +from bisect import bisect_right from dataclasses import dataclass from datetime import date, datetime from decimal import ROUND_CEILING, ROUND_FLOOR, Decimal, localcontext @@ -366,20 +367,33 @@ def query_numeric_histogram(task, node, column, column_type, min_value, max_valu def query_datetime_histogram(task, node, column, min_value, max_value): + def bounded_edges(start, terminal, interval): + # Each branch limits its bucket count before reaching this helper + # (daily <= 61, monthly <= 49, yearly <= 51). Iteration avoids an + # overflowing final relativedelta at date.max without a large range. + edges = [start] + while edges[-1] < terminal: + try: + next_edge = edges[-1] + interval + except (OverflowError, ValueError): + next_edge = date.max + edges.append(min(next_edge, terminal)) + return edges + days_delta = (max_value - min_value).days print(max_value, min_value, days_delta) # _type = None if days_delta > 365 * 4: _type = "yearly" dmin = date(min_value.year, 1, 1) - if max_value.year < 3000: + if max_value.year < date.max.year: dmax = date(max_value.year, 1, 1) + relativedelta(years=+1) else: - dmax = date(3000, 1, 1) - interval_years = math.ceil((dmax.year - dmin.year) / 50) + dmax = date.max + interval_years = max(1, math.ceil((dmax.year - dmin.year) / 50)) interval = relativedelta(years=+interval_years) - num_buckets = math.ceil((dmax.year - dmin.year) / interval.years) - bin_edges = [dmin + interval * i for i in range(num_buckets + 1)] + bin_edges = bounded_edges(dmin, dmax, interval) + num_buckets = len(bin_edges) - 1 sql = f""" SELECT {{{{ date_trunc("year", "{column}") }}}} as year, @@ -393,13 +407,12 @@ def query_datetime_histogram(task, node, column, min_value, max_value): _type = "monthly" interval = relativedelta(months=+1) dmin = date(min_value.year, min_value.month, 1) - if max_value.year < 3000: + if max_value.year < date.max.year or max_value.month < 12: dmax = date(max_value.year, max_value.month, 1) + interval else: - dmax = date(3000, 1, 1) - period = relativedelta(dmax, dmin) - num_buckets = period.years * 12 + period.months - bin_edges = [dmin + relativedelta(months=i) for i in range(num_buckets + 1)] + dmax = date.max + bin_edges = bounded_edges(dmin, dmax, interval) + num_buckets = len(bin_edges) - 1 sql = f""" SELECT {{{{ date_trunc("month", "{column}") }}}} as month, @@ -413,12 +426,16 @@ def query_datetime_histogram(task, node, column, min_value, max_value): _type = "daily" interval = relativedelta(days=+1) dmin = date(min_value.year, min_value.month, min_value.day) - if max_value.year < 3000: + if max_value < date.max: dmax = date(max_value.year, max_value.month, max_value.day) + interval else: - dmax = date(3000, 1, 1) - num_buckets = (dmax - dmin).days - bin_edges = [dmin + interval * i for i in range(num_buckets + 1)] + dmax = date.max + if dmin == dmax: + # date.max has no representable successor. Use the preceding day + # as the sole interval's left edge and close it at date.max. + dmin -= interval + bin_edges = bounded_edges(dmin, dmax, interval) + num_buckets = len(bin_edges) - 1 sql = f""" SELECT {{{{ date_trunc("day", "{column}") }}}} as day, @@ -448,15 +465,15 @@ def query_datetime_histogram(task, node, column, min_value, max_value): def build_result(query_result): if query_result is None: - return {} + return {"counts": []} counts = [0] * num_buckets for value, count in query_result.rows: edge_value = value.date() if isinstance(value, datetime) else value - if _type == "yearly": - index = (edge_value.year - dmin.year) // interval.years - else: - index = bin_edges.index(edge_value) + index = bisect_right(bin_edges, edge_value) - 1 + if index == num_buckets and edge_value == bin_edges[-1]: + # The date.max sentinel closes the last interval inclusively. + index -= 1 if index < 0 or index >= num_buckets: raise ValueError(f"Histogram date {edge_value} is outside the computed edge domain") counts[index] += count diff --git a/tests/fixtures/histogram_type_policy.json b/tests/fixtures/histogram_type_policy.json index 646ae2344..200fced7d 100644 --- a/tests/fixtures/histogram_type_policy.json +++ b/tests/fixtures/histogram_type_policy.json @@ -15,13 +15,29 @@ {"type": "TIME WITH TIME ZONE(6)", "backend_supported": false, "picker_supported": false}, {"type": "DATE", "backend_supported": true, "picker_supported": false}, {"type": "date", "backend_supported": true, "picker_supported": false}, + {"type": " DATE ( 6 ) ", "backend_supported": true, "picker_supported": false}, + {"type": "DATETIME", "backend_supported": true, "picker_supported": false}, + {"type": "datetime ( 6 )", "backend_supported": true, "picker_supported": false}, {"type": "TIMESTAMP", "backend_supported": true, "picker_supported": false}, {"type": "TIMESTAMP(6)", "backend_supported": true, "picker_supported": false}, + {"type": " timestamp ( 6 ) ", "backend_supported": true, "picker_supported": false}, + {"type": "YEAR", "backend_supported": true, "picker_supported": false}, + {"type": " year ( 4 ) ", "backend_supported": true, "picker_supported": false}, + {"type": "DATETIME2(7)", "backend_supported": true, "picker_supported": false}, + {"type": "SMALLDATETIME ( 3 )", "backend_supported": true, "picker_supported": false}, + {"type": "DATETIMEOFFSET(7)", "backend_supported": true, "picker_supported": false}, + {"type": "INTERVAL", "backend_supported": true, "picker_supported": false}, + {"type": " interval ( 6 ) ", "backend_supported": true, "picker_supported": false}, {"type": "TIMESTAMPTZ", "backend_supported": true, "picker_supported": false}, {"type": "TIMESTAMPTZ(6)", "backend_supported": true, "picker_supported": false}, {"type": "TIMESTAMP WITH TIME ZONE", "backend_supported": true, "picker_supported": false}, {"type": "timestamp without time zone", "backend_supported": true, "picker_supported": false}, {"type": " timestamp ( 6 ) with time zone ", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP WITHOUT TIME ZONE(6)", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP WITH LOCAL TIME ZONE(6)", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP_LTZ ( 9 )", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP_NTZ(9)", "backend_supported": true, "picker_supported": false}, + {"type": "TIMESTAMP_TZ ( 9 )", "backend_supported": true, "picker_supported": false}, {"type": "TIMEOUT", "backend_supported": true, "picker_supported": true}, {"type": "MYSTERY_DECIMAL", "backend_supported": true, "picker_supported": true} ] diff --git a/tests/tasks/test_histogram.py b/tests/tasks/test_histogram.py index ba00ff744..a921b763f 100644 --- a/tests/tasks/test_histogram.py +++ b/tests/tasks/test_histogram.py @@ -1,5 +1,5 @@ import json -from datetime import date, datetime +from datetime import date, datetime, timedelta from decimal import Decimal from pathlib import Path from types import SimpleNamespace @@ -211,10 +211,97 @@ def test_datetime_histogram_preserves_the_successful_side_when_one_query_fails() ) assert bin_edges == [date(2026, 1, 1), date(2026, 1, 2)] - assert base == {} + assert base == {"counts": []} assert current == {"counts": [4]} +def test_datetime_histogram_task_returns_a_stable_partial_payload_when_one_query_fails(monkeypatch): + """Catch a handled side failure omitting the public counts/total fields.""" + + class PartialDatetimeHistogramTask(HistogramDiffTask): + def execute_sql(self, sql, *, base): + if "MIN(created_at)" in sql: + return [(date(2026, 1, 1), date(2026, 1, 1), 4 if base else 7)] + if base: + raise RuntimeError("base warehouse query failed") + return HistogramRows([(date(2026, 1, 1), 7)]) + + def check_cancel(self): + return None + + adapter = MagicMock() + monkeypatch.setattr(histogram, "default_context", lambda: SimpleNamespace(adapter=adapter)) + task = PartialDatetimeHistogramTask( + { + "model": "orders", + "column_name": "created_at", + "column_type": "TIMESTAMP", + } + ) + + assert task.execute() == { + "base": {"counts": [], "total": 4}, + "current": {"counts": [7], "total": 7}, + "min": date(2026, 1, 1), + "max": date(2026, 1, 1), + "bin_edges": [date(2026, 1, 1), date(2026, 1, 2)], + "labels": None, + } + + +@pytest.mark.parametrize( + ("extreme", "expected_edges"), + [ + (date(3000, 6, 15), [date(3000, 6, 15), date(3000, 6, 16)]), + (date.max, [date.max - timedelta(days=1), date.max]), + ], +) +def test_datetime_histogram_extreme_daily_edges_stay_monotonic_and_bounded(extreme, expected_edges): + """Catch the year-3000 overflow guard reversing edges or allocating an unbounded range.""" + task = DatetimeHistogramQueryTask( + base_rows=[(extreme, 3)], + current_rows=[(extreme, 5)], + ) + + base, current, bin_edges = histogram.query_datetime_histogram( + task, + "orders", + "created_at", + extreme, + extreme, + ) + + assert bin_edges == expected_edges + assert len(bin_edges) == 2 + assert bin_edges[0] < bin_edges[-1] + assert bin_edges[0] <= extreme <= bin_edges[-1] + assert base == {"counts": [3]} + assert current == {"counts": [5]} + + +def test_datetime_histogram_date_max_yearly_edges_remain_bounded(): + """Catch terminal-date handling allocating by day or capping valid years at 3000.""" + task = DatetimeHistogramQueryTask( + base_rows=[(date(9999, 1, 1), 3)], + current_rows=[(date(1900, 1, 1), 5)], + ) + + base, current, bin_edges = histogram.query_datetime_histogram( + task, + "orders", + "created_at", + date(1900, 1, 1), + date.max, + ) + + assert bin_edges[0] == date(1900, 1, 1) + assert bin_edges[-1] == date.max + assert len(bin_edges) <= 52 + assert all(left < right for left, right in zip(bin_edges, bin_edges[1:])) + assert sum(base["counts"]) == 3 + assert sum(current["counts"]) == 5 + + class DecimalExtremaHistogramTask(HistogramDiffTask): def __init__(self, minimum, maximum, column_type="NUMERIC"): super().__init__({"model": "numbers", "column_name": "amount", "column_type": column_type, "num_bins": 1})