From d7c6a09231b2ce82abe851bd367f55454f9ef9a1 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Wed, 7 Oct 2026 10:38:37 -0400 Subject: [PATCH 1/2] test(diff): failing tests for skipping the compare view's hidden columns in Buckaroo's stats The compare grid is three to four times as wide as the entries it compares, and a third to a half of its columns are hidden: the before-value of each column, its equality sentinel, and membership. Buckaroo runs a batch aggregate and a histogram query for every one of them, and nothing reads a stat of any. These tests describe sending them as skip_stat_columns on the diff's /load_expr: - diff_skip_stat_columns(overrides) is exactly the hidden columns, sorted. A column that changed to an incomparable dtype (#68) keeps its before-value visible and is not in it, and neither is a key. - The diff route posts it as a list, over the file it hands Buckaroo, and changes no other field of the body. No visible column of the file is in it. - Over a real Buckaroo dataflow built from the posted body: no view draws a skipped column, every color_map rule reads the histogram of a column that keeps its stats, every view's display config is the same with and without the list, and the stat pipeline runs fewer queries with it on a cold stat cache while visible columns keep their histograms. All ten fail on this commit: the function does not exist and the body has no skip_stat_columns. Co-Authored-By: Claude Sonnet 5.5 --- tests/test_diff_skip_stats.py | 288 ++++++++++++++++++++++++++++++++++ 1 file changed, 288 insertions(+) create mode 100644 tests/test_diff_skip_stats.py diff --git a/tests/test_diff_skip_stats.py b/tests/test_diff_skip_stats.py new file mode 100644 index 0000000..c11dcec --- /dev/null +++ b/tests/test_diff_skip_stats.py @@ -0,0 +1,288 @@ +"""The live diff tells Buckaroo not to compute summary stats for the compare columns that no display shows a stat of. + +Buckaroo runs a batch aggregate and one histogram query per column of the grid it is handed. The compare grid is three +to four times as wide as the entries it compares, and a third to a half of its columns are hidden: the before-value of +each column, its equality sentinel, and ``membership``. Nothing reads a stat of those, so they are sent as +``skip_stat_columns`` (``plans/diff-performance-proposals.md``, B1). +""" + +from __future__ import annotations + +import datetime as dt +from pathlib import Path + +import pyarrow as pa +import pyarrow.parquet as pq +import pytest +import xorq.api as xo + +from tests.test_diff_snapshot import _cheap_pair, _diff_files, _open_diff, _worthy_pair + +# What a diff of ``shoe_sales`` V1 to V2 (``region``, ``total``, ``n``) hides: ``membership``, the before-value and +# the equality sentinel of each value column, and the file's ``__row_order``. +SHOE_SALES_HIDDEN = {"membership", "total", "total_eq", "n", "n_eq", "__row_order"} + + +def _parquet_expr(tmp_path: Path, name: str, table: pa.Table): + path = tmp_path / f"{name}.parquet" + pq.write_table(table, path) + return xo.deferred_read_parquet(str(path)) + + +def _hidden(overrides: dict) -> set[str]: + return {c for c, cfg in overrides.items() if cfg.get("merge_rule") == "hidden"} + + +def _body(project: str, alias: str) -> dict: + """The one ``/load_expr`` body the diff route posts for V1 to V2 of *alias*.""" + posted: list[dict] = [] + _open_diff(project, alias, posted) + assert len(posted) == 1 + return posted[0] + + +def _compare(tmp_path: Path, keys: list[str], a_cols: dict, b_cols: dict): + from tallyman_companion.diff import build_compare_expr, strip_live_diff_color + + a = _parquet_expr(tmp_path, "a", pa.table(a_cols)) + b = _parquet_expr(tmp_path, "b", pa.table(b_cols)) + expr, overrides = build_compare_expr(a, b, keys) + return expr, strip_live_diff_color(overrides) + + +# --------------------------------------------------------------------------- +# which columns +# --------------------------------------------------------------------------- + + +def test_skip_list_is_exactly_the_columns_the_overrides_hide(tmp_path: Path): + """The before-value and the equality sentinel of each comparable column, and ``membership``. A column whose dtype + changed to an incomparable one (#68) is shown side by side, so its before-value is visible and not skipped.""" + from tallyman_companion.diff import diff_skip_stat_columns + + expr, overrides = _compare( + tmp_path, + ["id"], + { + "id": pa.array([1, 2, 3], pa.int64()), + "amt": pa.array([10, 20, 30], pa.int32()), + "price": pa.array([1.5, 2.5, 3.5], pa.float64()), + "name": pa.array(["a", "b", "c"], pa.string()), + "issue_date": pa.array(["2024-01-01", "2024-02-01", "2024-03-01"], pa.string()), + "removed": pa.array([1, 1, 1], pa.int64()), + }, + { + "id": pa.array([2, 3, 4], pa.int64()), + "amt": pa.array([21, 30, 40], pa.int64()), + "price": pa.array([2.5, 3.0, 4.5], pa.float64()), + "name": pa.array(["b", "x", "d"], pa.string()), + "issue_date": pa.array([dt.date(2024, 2, 1), dt.date(2024, 3, 2), dt.date(2024, 4, 1)], pa.date32()), + }, + ) + + skip = diff_skip_stat_columns(overrides) + + assert isinstance(skip, list) + assert skip == sorted(set(skip)), "sorted and without repeats, so the posted body is stable" + assert set(skip) == {"membership", "amt", "amt_eq", "price", "price_eq", "name", "name_eq"} + visible = set(expr.columns) - set(skip) + assert { + "id", + "amt_v2", + "amt_pct_delta", + "amt_abs_delta", + "price_v2", + "price_pct_delta", + "price_abs_delta", + } <= visible + assert {"name_v2", "issue_date", "issue_date_v2", "removed"} <= visible + assert set(skip) <= set(expr.columns), "a skipped name that the grid does not have would be a silent no-op" + + +def test_a_key_that_changed_dtype_is_never_skipped(tmp_path: Path): + """#68: both sides of such a key are cast to string for the join, and the key stays the grid's colored column.""" + from tallyman_companion.diff import diff_skip_stat_columns + + expr, overrides = _compare( + tmp_path, + ["issue_date"], + {"issue_date": pa.array(["2024-01-01", "2024-02-01"], pa.string()), "amt": pa.array([10, 20], pa.int64())}, + { + "issue_date": pa.array([dt.date(2024, 1, 1), dt.date(2024, 3, 1)], pa.date32()), + "amt": pa.array([15, 25], pa.int64()), + }, + ) + + skip = diff_skip_stat_columns(overrides) + + assert set(skip) == {"membership", "amt", "amt_eq"} + assert "issue_date" in expr.columns and "issue_date" not in skip + + +def test_no_overrides_hide_nothing(): + from tallyman_companion.diff import diff_skip_stat_columns + + assert diff_skip_stat_columns({}) == [] + assert diff_skip_stat_columns({"a": {"header_name": "A"}, "b": {}}) == [] + + +# --------------------------------------------------------------------------- +# what the route sends +# --------------------------------------------------------------------------- + + +def test_diff_load_posts_the_skip_list_as_a_list(project: str, orders_src: str, monkeypatch): + """Buckaroo turns the field into a set, so a bare string would become a set of letters and skip nothing.""" + monkeypatch.setenv("TALLYMAN_PROJECT", project) + _worthy_pair(project) + + skip = _body(project, "shoe_sales")["skip_stat_columns"] + + assert isinstance(skip, list) + assert set(skip) == SHOE_SALES_HIDDEN + assert len(skip) == len(set(skip)) + + +@pytest.mark.parametrize("pair", [_worthy_pair, _cheap_pair], ids=["worthy", "cheap"]) +def test_the_posted_skip_list_is_the_hidden_columns_of_the_posted_overrides_and_no_other( + project: str, orders_src: str, monkeypatch, pair +): + """Over every column of the file Buckaroo is handed: hidden ones are skipped, and no visible one is.""" + monkeypatch.setenv("TALLYMAN_PROJECT", project) + pair(project) + alias = "shoe_sales" if pair is _worthy_pair else "rows" + + body = _body(project, alias) + + [path] = _diff_files(project) + columns = set(pq.read_schema(path).names) + hidden = _hidden(body["column_config_overrides"]) & columns + assert hidden, "the compare grid always hides membership" + assert set(body["skip_stat_columns"]) == hidden + assert columns - hidden, "something stays visible" + assert not (columns - hidden) & set(body["skip_stat_columns"]) + + +def test_the_other_fields_of_the_load_are_what_they_were(project: str, orders_src: str, monkeypatch): + """The skip list is added to the body; the session, the stat cache, the data id, the stats delivery, the klasses + and the overrides stay.""" + monkeypatch.setenv("TALLYMAN_PROJECT", project) + _worthy_pair(project) + + body = _body(project, "shoe_sales") + + assert set(body) == { + "session", + "build_dir", + "no_browser", + "column_config_overrides", + "skip_stat_columns", + "cache_storage_path", + "data_id", + "stats_delivery", + "extra_grid_config", + "project_root", + } + assert body["column_config_overrides"]["membership"] == {"merge_rule": "hidden"} + + +# --------------------------------------------------------------------------- +# what Buckaroo does with it, over the file the route hands it +# --------------------------------------------------------------------------- + + +def _dataflow(body: dict, skip, cache_dir: Path): + """The dataflow Buckaroo's ``LoadExprHandler`` builds for *body*, with *skip* as its ``skip_stat_columns``.""" + from buckaroo.server import xorq_loading + + expr = xorq_loading.load_expr_build_dir(body["build_dir"]) + klasses = xorq_loading.load_project_display_klasses(body["project_root"]) + return xorq_loading.XorqServerDataflow( + expr, + skip_main_serial=True, + extra_klasses=klasses, + cache_storage_path=str(cache_dir), + column_config_overrides=body["column_config_overrides"], + skip_stat_columns=skip, + ) + + +def _column_defs(display_arg: dict) -> list[dict]: + return display_arg["df_viewer_config"]["column_config"] + + +@pytest.fixture +def shoe_sales_body(project: str, orders_src: str, monkeypatch) -> dict: + monkeypatch.setenv("TALLYMAN_PROJECT", project) + _worthy_pair(project) + return _body(project, "shoe_sales") + + +def test_no_view_shows_a_skipped_column_or_reads_its_stats(shoe_sales_body: dict, tmp_path: Path): + """The guard for what the skip list may not take away: no view draws a skipped column, and every ``color_map`` + rule reads the histogram of a column that keeps its stats, since the JS reads ``histogram_stats[val_column]``.""" + skip = set(shoe_sales_body["skip_stat_columns"]) + assert skip + df = _dataflow(shoe_sales_body, shoe_sales_body["skip_stat_columns"], tmp_path / "skip") + alias_to_name = {alias: stats["orig_col_name"] for alias, stats in df.merged_sd.items()} + + color_maps = 0 + assert {"main", "detailed_pct", "detailed_absolute", "summary"} <= set(df.df_display_args) + for view, arg in df.df_display_args.items(): + for col in _column_defs(arg): + if col.get("col_name") in alias_to_name: + assert alias_to_name[col["col_name"]] not in skip, f"{view} draws a skipped column" + cmc = col.get("color_map_config") or {} + if cmc.get("color_rule") == "color_map": + color_maps += 1 + assert alias_to_name[cmc["val_column"]] not in skip, f"{view} colors by a skipped column's histogram" + assert color_maps, "no view colored by a histogram, so this guard checked nothing" + + +def test_display_config_is_the_same_with_and_without_the_skip_list(shoe_sales_body: dict, tmp_path: Path): + """What the grid draws, in every view, and the stats it draws it from for each column that is not skipped.""" + skip = shoe_sales_body["skip_stat_columns"] + assert skip + with_skip = _dataflow(shoe_sales_body, skip, tmp_path / "with") + without = _dataflow(shoe_sales_body, [], tmp_path / "without") + + assert with_skip.df_display_args == without.df_display_args + + def stats(df): + return { + stats["orig_col_name"]: {k: v for k, v in stats.items() if k != "histogram"} + for stats in df.merged_sd.values() + if stats["orig_col_name"] not in skip + } + + assert stats(with_skip) == stats(without) + + +def test_skipped_columns_get_no_stat_queries_and_the_rest_keep_their_histograms( + shoe_sales_body: dict, tmp_path: Path, monkeypatch +): + """A cold stat cache both times. Also guards a Buckaroo that stops honoring the field.""" + from buckaroo.pluggable_analysis_framework.xorq_stat_pipeline import XorqStatPipeline + + queries: list[int] = [] + execute = XorqStatPipeline._execute + + def counting(self, query): + queries.append(1) + return execute(self, query) + + monkeypatch.setattr(XorqStatPipeline, "_execute", counting) + skip = shoe_sales_body["skip_stat_columns"] + + _dataflow(shoe_sales_body, [], tmp_path / "without") + n_without, queries[:] = len(queries), [] + with_skip = _dataflow(shoe_sales_body, skip, tmp_path / "with") + n_with = len(queries) + + assert n_with < n_without + by_name = {stats["orig_col_name"]: stats for stats in with_skip.merged_sd.values()} + assert all("histogram" not in by_name[c] for c in skip) + numeric_visible = ["total_v2", "total_pct_delta", "n_v2", "n_abs_delta"] + assert all("histogram" in by_name[c] for c in numeric_visible) + assert {c for c in by_name if "histogram" in by_name[c]} >= set(numeric_visible) + assert all(by_name[c]["dtype"] and by_name[c]["length"] for c in skip), "name, dtype and length are kept" From 4b735092a0a782cab886a3c1bac9b7085f8b6f90 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Wed, 7 Oct 2026 10:57:24 -0400 Subject: [PATCH 2/2] perf(diff): skip summary stats for the compare grid's hidden columns The diff's /load_expr now carries skip_stat_columns: the compare columns that the grid's overrides hide. Those are each column's before-value, its equality sentinel, membership, and the file's __row_order. Buckaroo ran a batch aggregate and a histogram query for every one of them, and nothing reads a stat of any: the key and equality colors read row values, the before-value tooltip reads a cell, and only the delta columns' min, max and histogram bins are read, by the display klasses and by color_map. Those are never hidden. diff_skip_stat_columns(overrides) reads the hidden set off the same overrides the grid is configured with, so the two cannot drift, and returns it sorted. A skipped column keeps its name, dtype and length. The display config is the same with and without the list, and no visible column is in it. Buckaroo honors the field in 0.15.9 and 0.15.10 alike. Co-Authored-By: Claude Sonnet 5.5 --- docs/architecture-new.md | 13 ++++++++----- docs/architecture.md | 10 ++++++---- plans/diff-performance-proposals.md | 4 ++-- src/tallyman_companion/app.py | 2 ++ src/tallyman_companion/diff.py | 15 +++++++++++++++ 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/docs/architecture-new.md b/docs/architecture-new.md index 37dfc8f..f935dfb 100644 --- a/docs/architecture-new.md +++ b/docs/architecture-new.md @@ -739,11 +739,14 @@ and `_abs_delta` columns. The route runs that join once rows each side reads and the join keys (`diff_snapshot.diff_identity`), so a heal gives another file. Buckaroo is handed a view build of that file, one read and no history of the join, as session `diff---`, and runs its -statistics, pages and searches over the file. With a session loaded the route -skips `full_diff`'s queries: the code and schema diffs are file reads and the -three counts are a group-by over the file. Without Buckaroo, a key, or a file -or session that loads, the page gets `full_diff`'s summaries. Diff sessions, -unlike entry sessions, are remembered until Buckaroo restarts. +statistics, pages and searches over the file. The post lists the columns the +grid hides (each column's before-value and `_eq`, `membership`, `__row_order`) as +`skip_stat_columns` (`tallyman_companion.diff.diff_skip_stat_columns`), so +Buckaroo runs no statistic queries for columns no view draws. With a session +loaded the route skips `full_diff`'s queries: the code and schema diffs are file +reads and the three counts are a group-by over the file. Without Buckaroo, a +key, or a file or session that loads, the page gets `full_diff`'s summaries. +Diff sessions, unlike entry sessions, are remembered until Buckaroo restarts. **Promoted diffs.** `catalog_promote_diff` and the companion's promote route save a diff as an entry whose generated recipe is: diff --git a/docs/architecture.md b/docs/architecture.md index 615564c..52b9137 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -788,10 +788,12 @@ left behind, survives a failed re-add. 4. Buckaroo is handed a view build of that file, one read and no history of the join, as session `diff---`, with statistics cached per pair under `diff_stat_cache/`. Its statistics, pages, sorts and searches all run - over the file. Without Buckaroo, or without a key, or when the file or the - session fails, the page gets the code, schema, statistics, head and keyed - diffs from `full_diff`, which still include `__row_order` as a data column - (#200). + over the file, and the columns the grid hides (before-values, `_eq`, + `membership`, `__row_order`) are posted as `skip_stat_columns`, so no + statistic queries run for them. Without Buckaroo, or without a key, or when + the file or the session fails, the page gets the code, schema, statistics, + head and keyed diffs from `full_diff`, which still include `__row_order` as a + data column (#200). `catalog_promote_diff` and the diff page's promote button turn a diff into an entry of its own, whose recipe calls `build_diff_expr(a_hash, b_hash, keys)`. diff --git a/plans/diff-performance-proposals.md b/plans/diff-performance-proposals.md index 1a35fa0..cb6a7af 100644 --- a/plans/diff-performance-proposals.md +++ b/plans/diff-performance-proposals.md @@ -1,6 +1,6 @@ # Diff view: where the time goes, and proposals to cut it -Status: analysis and proposals (2026-10-03). Section 4a is implemented in #309; nothing else here is. +Status: analysis and proposals (2026-10-03). Section 4a is implemented in #309 and B1 in #312; nothing else here is. Measurements were taken in-process with a scratch script that replays `api_diff_data` stage by stage and then replays what Buckaroo's `LoadExprHandler` does with the posted build (the same proxy @@ -203,7 +203,7 @@ was up and a swap storm would have hit it. **B. Cut Buckaroo's stat work on the diff.** -- B1. Pass `skip_stat_columns` for the hidden before-value `{col}` columns, +- B1 (implemented in #312). Pass `skip_stat_columns` for the hidden before-value `{col}` columns, every `{col}_eq`, and `membership`. Buckaroo supports it today, and the handler comment names "a diff reusing each source column's stats" as its purpose. This removes roughly half of the per-column histogram queries. diff --git a/src/tallyman_companion/app.py b/src/tallyman_companion/app.py index 004ecdf..5ec948c 100644 --- a/src/tallyman_companion/app.py +++ b/src/tallyman_companion/app.py @@ -18,6 +18,7 @@ from sse_starlette.sse import EventSourceResponse from tallyman_companion.buckaroo_lifecycle import BuckarooManager +from tallyman_companion.diff import diff_skip_stat_columns from tallyman_companion.diff_snapshot import open_diff_view, view_diff from tallyman_core import ( ENTRY_BUILD_DIRNAME, @@ -457,6 +458,7 @@ def _load_compare_view( "build_dir": str(view.build_dir), "no_browser": True, "column_config_overrides": view.column_config_overrides, + "skip_stat_columns": diff_skip_stat_columns(view.column_config_overrides), "cache_storage_path": str(stat_cache), "data_id": view.identity, "stats_delivery": "deferred", diff --git a/src/tallyman_companion/diff.py b/src/tallyman_companion/diff.py index 548d35b..48dd69a 100644 --- a/src/tallyman_companion/diff.py +++ b/src/tallyman_companion/diff.py @@ -139,6 +139,21 @@ def strip_live_diff_color(overrides: dict) -> dict: return out +def diff_skip_stat_columns(overrides: dict) -> list[str]: + """The compare grid's columns that no display shows a stat of: the ones its overrides hide. + + Buckaroo runs a batch aggregate and one histogram query per column. A hidden column is never drawn, and no stat + of it is read by anything that is: the key and equality colors read row values (``color_categorical``'s + ``val_column`` is a cell), the before-value tooltip reads a cell, and only the delta columns' ``min``, ``max`` and + histogram bins are read, by ``diff_extras/display`` and ``color_map``, and those are never hidden. A skipped + column keeps its name, dtype and length and gets no stat queries. + + Never put a visible column here: a column with no stats loses its displayer. Reading the hidden set off the same + overrides the grid is configured with keeps the two from drifting. Sorted, so the posted body is stable. + """ + return sorted(col for col, cfg in overrides.items() if isinstance(cfg, dict) and cfg.get("merge_rule") == "hidden") + + def _marker_names(taken: set[str]) -> tuple[str, str]: """Names for the two side markers that no column of either side already has."""