diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index 2543c8e..d10f9b9 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -423,6 +423,52 @@ feature work**, and so we can tell the difference between "this is awkward" and keep both P34 regression tests green — they pin exactly the two behaviours the shared resolver has to reproduce. +### R12 — Two aggregate registries, nine functions implemented twice +- **Status:** 🔴 OPEN — filed 2026-09-06 by [P41](SQL_PARITY.md#p41) +- **Where:** `src/sql/aggregates/` (old, `AggregateRegistry`) and + `src/sql/aggregate_functions/` (new, `AggregateFunctionRegistry`). Dispatch is + in `ArithmeticEvaluator`, which holds both (`arithmetic_evaluator.rs:24-25`, + the fields commented *"old registry (being phased out)"* and *"new registry"*) + and checks the new one **first** at both call sites (`:637`, `:769`). +- **Observed:** the migration adds to the new registry without removing from the + old, so the two overlap and the old copy is unreachable for everything in the + intersection. As of filing: + + | | Functions | + |---|---| + | **Both — old copy is dead** | `AVG`, `MIN`, `MAX`, `STDDEV`, `VARIANCE`, `MEDIAN`, `MODE`, `PERCENTILE`, `STRING_AGG` | + | **New only** | `COUNT`, `COUNT_STAR`, `SUM` — the three done properly, commented out in the old list | + | **Old only — live** | `STDDEV_POP`, `STDDEV_SAMP`, `VAR_POP`, `VAR_SAMP`, and the analytics set (`DELTAS`, `SUMS`, `MAVG`, `PCT_CHANGE`, `RANK`, `CUMMAX`, `CUMMIN`) | + + So the old registry is neither dead nor live — it is both, per function, and + nothing in either file says which. +- **Impact: a fix can land in the wrong one and pass its own tests.** That is not + hypothetical; it is how [P41](SQL_PARITY.md#p41) went. The finding named + `ModeState` (old), the fix was written and unit-tested there, and the repro + still flipped between runs, because the live `MODE` is `CollectorState` in the + new registry. Worse, the two implementations *disagree on semantics*: the dead + one keys on a string rendering and returns the original `DataValue`; the live + one collects `Vec` and so rejects non-numeric input and returns a float + for integers ([P42](SQL_PARITY.md#p42)). The better implementation is the + unreachable one. +- **Same shape as [R8](#r8) and [R11](#r11)**, with the failure mode of R8 (a + parallel stack with its own semantics) and the trap of R11 (the copy that runs + is the wrong one). It is worse than either in one respect: R8's legacy stack is + reachable only from tests, and R11's duplication is 3000 lines apart in *one* + file. Here both registries are genuinely live, and which one serves a given + function is invisible at every call site. +- **Decision:** converge on the new registry, but **not as one change**. Order: + (1) make the overlap harmless — assert at construction that the two key sets + are disjoint, or simply delete the nine shadowed entries from the old list, + which is a provable no-op since they are unreachable today; (2) port the + old-only functions (`*_POP`/`*_SAMP` and analytics) across; (3) delete the old + registry. Step 1 is the one worth doing soon and is small — it is what stops + the next P41. +- **Guard rail:** step 1 must not change behaviour, so the acceptance test is + parity staying at exactly its current AGREE count, plus the FORMAL examples. + [P42](SQL_PARITY.md#p42) is the natural companion to step 2 — porting `MODE`'s + type handling from the dead implementation to the live one is most of that fix. + --- ## Sequencing @@ -446,6 +492,7 @@ R5 dead code ─────── opportunistic R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a lull R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129) R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run +R12 aggregate registries ─ independent; step 1 is a provable no-op, do it before the next aggregate fix ``` **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator @@ -480,3 +527,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 2026-08-30 | `RANGE` window frames given peer-group semantics (`OrderedPartition::peer_bounds`); sorting and peer detection unified on one comparator. Closes parity P24 — 129 → **133 AGREE**; new finding P33 (`RANGE` with a numeric offset) now a deliberate hard error rather than a silent ROWS answer | — | | 2026-08-30 | P34 fixed: `ORDER BY "col.with.dot"` no longer strips a quoted identifier at the dot. R11 filed — ORDER BY still resolves columns with its own copy of `resolve_column_index` rather than the canonical one | — | | 2026-09-04 | P40 filed from field use: a generator's args are evaluated against DUAL (`statement_executor.rs` has no `from_function` case), so no column reference resolves in `FROM SPLIT(col, …)`. Cross-linked here — R1's missing table-function variant is the reason generators sit on the legacy path | — | +| 2026-09-06 | R12 filed by parity P41: two aggregate registries, nine functions implemented twice with the newer one shadowing the older. P41's fix was written against the dead copy first and changed nothing — the entry records the disjointness assertion as step 1 | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 2cc65ea..db819ca 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -65,14 +65,16 @@ session apiece to fix properly, so **discovery is paused and the effort moves to picking them off**. Widen the corpus again when the open list is short, or opportunistically when a fix needs a case that doesn't exist yet. -Corpus coverage today: tiers 01–10, **177 cases** (152 AGREE / 10 DIFFER / -12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-05, after the NULL-ordering -slice closed [P13](#p13) stage 2 and [P17](#p17) together — eleven cases in one -change, the largest single movement so far). **Tier 10 -(aggregate & NULL edges) is deliberately partial** — it holds the P14 and P18–P20 cases and their baselines, -but was never built out the way tiers 08 and 09 were. Finish it during a lull; -the aggregate-function surface (`STDDEV`, `DISTINCT` aggregates, `FILTER`, -empty-vs-all-NULL distinctions) is largely unexamined. +Corpus coverage today: tiers 01–10, **181 cases** (156 AGREE / 10 DIFFER / +12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-06, after [P41](#p41) added four +MODE cases). The largest single movement so far remains the 2026-09-05 +NULL-ordering slice, which closed [P13](#p13) stage 2 and [P17](#p17) together — +eleven cases in one change. **Tier 10 (aggregate & NULL edges) is still +deliberately partial** — it holds the P14, P18–P20 and P41 cases and their +baselines, but was never built out the way tiers 08 and 09 were. Finish it during +a lull; the aggregate-function surface (`STDDEV`, `DISTINCT` aggregates, +`FILTER`, empty-vs-all-NULL distinctions) is largely unexamined, and P41 showed +that surface is thinner than it looks. Suggested fix order, by silent blast radius: @@ -88,11 +90,12 @@ Suggested fix order, by silent blast radius: | ~~9a~~ | ~~[P16](#p16) `ORDER BY ` ignored~~ | ✅ **Fixed 2026-08-31** — 134 → 139 AGREE. The literal was being promoted into a hidden *constant* column, so the sort ran on a column where every row tied | | ~~9c~~ | ~~[P17](#p17) + [P13](#p13) stage 2 — NULL ordering~~ | ✅ **Fixed 2026-09-05** — 141 → **152 AGREE**, eleven cases in one change. Both halves were the same comparator's NULL rule, so they were taken as one slice. The two sorts that disagreed with each other now *share* one function (`compare_for_order_by`), which is the part that stops the divergence recurring; the window site turned out to be sorting NULL as the **maximum** via a derived `PartialOrd`, not merely following a different rule | | ~~9d~~ | ~~[P37](#p37) window in `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-05** — corpus count unchanged, and that is the finding: the case is `OURS_ONLY` before *and* after, so the harness cannot see this fix or a future regression of it (first entry of that kind — the regression test is a Rust module). The filed root cause was wrong: `ExpressionLifter` *does* lift from `WHERE`. The real defect was one arm in the WHERE evaluator answering FALSE for any bare value used as a predicate — `WHERE true` returned zero rows too. It did **not** close [P15](#p15), which needs the opposite change | -| **NEXT** | [P41](#p41) `MODE` tie-break is random per run | **Silent, and it moves.** Found while verifying P37. Same input, same binary, different answer — six runs gave `0 1 1 0 0 1`. Worse than a wrong constant because no captured expectation can hold it, which is currently what blocks two example files from being promoted to FORMAL. Likely small: pick a total rule (smallest value wins) after checking whether the reference specifies one | -| 9b | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken | +| ~~9e~~ | ~~[P41](#p41) `MODE` tie-break is random per run~~ | ✅ **Fixed 2026-09-06** — 152 → **156 AGREE** (four new cases). Small, as predicted, but not where it was filed: the named `ModeState` was a *shadowed* implementation and fixing it moved nothing. Reference does specify a rule and it is **first-occurrence**, not the "smallest value wins" this row proposed. Unblocked both example files, now FORMAL. Spun off [P42](#p42), [P43](#p43), [R12](ENGINE_REFACTORING.md#r12) | +| **NEXT** | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken. Was row 9b | +| 9f | [P42](#p42) `MODE` is numeric-only | Companion to [R12](ENGINE_REFACTORING.md#r12), and cheap if taken with it: the shadowed implementation already handles non-numerics and preserves type, so the fix is largely to stop the live path throwing away what it knows. Also buys the corpus its clearest tie-break case | | 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32), [P38](#p38) | Hard errors — visible, so less urgent than any of the above | | 10b | [P39](#p39) `x/0` errors, voiding the whole statement | Hard error like row 10, but the only one whose blast radius is the *query* rather than the cell. Settle the four inconsistent call sites as one decision; it currently has no live probe (see the entry) | -| 11 | [P35](#p35), [P36](#p36) | Not parity obligations — a DuckDB extension and a naming difference. Decide *whether*, not just when | +| 11 | [P35](#p35), [P36](#p36), [P43](#p43) | Not parity obligations — a DuckDB extension, a naming difference, and a `RANGE` endpoint convention. Decide *whether*, not just when. P43 is the one with a migration cost attached, so it wants deciding before it accumulates more callers | | 12 | [P40](#p40) a generator's args can't reference columns | Hard error and loudly signposted, so last by blast radius — but it splits: the misleading *"may not support qualified column names"* message is a few lines and is the part that wastes the next person's afternoon. Take that alone; the explode feature can wait on the `UNNEST` decision | | — | [P27](#p27) `OR` in `JOIN ... ON` | **Reclassified 2026-08-08, re-scoped 2026-09-04 — possibly smaller than it was filed as.** The AST is still the blocker (`JoinCondition` is a `Vec` of AND-ed conditions with nowhere to put an `OR`), but the executor already evaluates expressions per row pair and already has a merged-row `cross_join`, so `INNER JOIN ON ` may lower to cross-join + the R10 WHERE evaluator. Do the timeboxed scoping pass in the entry before sequencing this | @@ -115,6 +118,25 @@ the fix; the entry describes the symptom that was looked for, not necessarily th defect. The same session's sweep for other consumers of the same route turned up [P31](#p31), a live zero-rows bug in the `--limit` flag that nobody had reported. +**A fourth lesson, from closing P41 (2026-09-06): the entry named a fix site, and +the fix site was dead code.** `ModeState` in `src/sql/aggregates/mod.rs` is +exactly what you find by grepping for MODE, and it is shadowed — a second, newer +aggregate registry is consulted first, so the live implementation is a different +tally in a different file ([R12](ENGINE_REFACTORING.md#r12)). The fix was applied, +the tests passed, and the repro still flipped between runs. Generalised: **an +entry's "Where" line is a lead, not a location. Confirm a fix site by changing it +and watching the symptom move.** Where two implementations of the same operator +exist, expect the *older-looking* one to be the dead one, since migrations here +add to the new registry without removing from the old. + +**A fifth, cheaper one from the same session: check the fixture, not just the +query.** P41's repro leaned on `RANGE(1,50)` being a 25/25 even/odd split. It is +— for us. DuckDB's `range` stop bound is exclusive, so the same query is 25/24 +there and the tie the repro depends on does not exist. That is now [P43](#p43), +and it was found only because a corpus case forces both engines to run the same +text. Any repro written against one engine carries assumptions about that +engine's builtins. + **Two lessons from closing P29/P30 (2026-08-08), both worth generalising:** 1. **Two findings filed as different classes were one bug.** P29 was "a parse @@ -1754,14 +1776,16 @@ out of date. plus one in tier 06 (`06_ctes_setops.toml`) for the shape that was actually hit. Expect `GAP`. ### P41 — `MODE` picks a tie-break winner at random, run to run - -- **Status:** 🔴 OPEN — **silent wrong answer, and nondeterministic** -- **Corpus:** none yet — see *Pinning it* below. -- **Observed:** `MODE` tallies into a `std::collections::HashMap` and takes the - highest count with **no tie-break rule** - (`ModeState`, `src/sql/aggregates/mod.rs`). When two or more values tie, the - winner is whichever the hash iteration order happens to surface. Six runs of - the *same binary*, same data: +- **Status:** 🟢 FIXED 2026-09-06 — branch `fix/p41-mode-tiebreak`. 177 → **181 + cases, 152 → 156 AGREE** (all four new, no bucket changes elsewhere) +- **Corpus:** `10_aggregate_nulls.toml :: mode_two_way_tie`, + `mode_tie_first_seen_not_smallest`, `mode_all_null`, `mode_grouped` — all AGREE. +- **Regression tests:** `mode_tie_break_tests` in + `src/sql/aggregate_functions/mod.rs` (the live path, driven through the + registry) and in `src/sql/aggregates/mod.rs` (the shadowed one) — six cases each. +- **Observed:** `MODE` tallied into a `HashMap` and took the highest count with + **no tie-break rule**. When two or more values tied, the winner was whichever + the hash iteration surfaced last. Six runs of the *same binary*, same data: ``` $ for i in 1 2 3 4 5 6; do sql-cli -q "WITH r AS (SELECT value % 2 AS pn @@ -1769,31 +1793,105 @@ out of date. 0 1 1 0 0 1 ``` - `RANGE(1,50)` is 25 even and 25 odd — a perfect tie — so both answers are - defensible and neither is stable. - - **Found:** 2026-09-05, while verifying the [P37](#p37) fix. It surfaced as three `examples/*.sql` files differing between the pre-fix and post-fix binaries; the queries involved have no `WHERE` clause at all, which is what prompted checking the same binary twice instead of blaming the change. -- **Why it matters more than a tie-break usually would.** Two example files sit - directly on it: `stats_examples.sql` on the 25/25 parity tie above, and - `statistical_analysis.sql` on all-count-1 ties across three columns. Both are - currently smoke tests. **Do not `--capture` an expectation for either while - this is open** — the captured value would be whichever way the coin landed, - and would then fail intermittently forever. That is the practical cost here: - it silently blocks two files from being promoted to FORMAL. - -- **Pinning it:** a corpus case needs a deterministic reference answer to - compare against, so check DuckDB's rule first — it may itself be - unspecified on ties, in which case the corpus is the wrong instrument and this - wants a Rust test asserting *stability* (same input, same answer) plus - whatever rule we choose. "Smallest value wins" is the obvious candidate: cheap, - total, and it makes both example files capturable. - -- **Related:** the same class as [P36](#p36) — behaviour that is unspecified - rather than wrong — but unlike P36 this one moves under you between runs. +- **The reference does specify a rule, and it is not the obvious one.** This + entry originally proposed *smallest value wins* — total, cheap, and wrong. + DuckDB breaks ties by **first occurrence in the input** + (`extension/core_functions/aggregate/holistic/mode.cpp`, which tracks a + `first_row` per distinct value and compares + `count > best.count || (count == best.count && first_row < best.first_row)`). + Probed directly to confirm: `(0,0,1,1)` → `0` but `(1,1,0,0)` → `1`; + `('b','b','a','a')` → `'b'`; `(5,3,9,1)` → `5`, not `1`. Stable across runs + including 1M-row parallel aggregation. NULLs ignored; all-NULL or empty → NULL, + which we already matched. + + So the rule is **earliest-seen wins**, per + [*follow the reference engine*](#where-the-standard-leaves-a-choice-open-follow-the-reference-engine). + Worth stating the trade-off plainly: earliest-seen is deterministic *given a + row order*, not a total order over values. That is weaker than "smallest wins" + would have been, and it is what the reference does. + +- **The filed location was the wrong one — a shadowed implementation.** This + entry named `ModeState` in `src/sql/aggregates/mod.rs`. Fixing it there changed + nothing: the repro was still `1 1 0 0 0 1 0 0` afterwards. There are **two** + aggregate registries, and `ArithmeticEvaluator` checks the newer one *first* + (`arithmetic_evaluator.rs:637`, `:769`), so the live `MODE` is + `CollectorState`/`CollectorFunction::Mode` in + `src/sql/aggregate_functions/mod.rs` — a completely separate tally, over + `f64::to_bits` keys, with the same missing tie-break. Both are fixed here; the + duplication itself is filed as [R12](ENGINE_REFACTORING.md#r12). + + This is the [P28](#p28) lesson recurring in a new form. There the write-up + inherited its probe's blind spot; here it inherited a *grep's* — the struct + named `ModeState` is not the code that runs `MODE`. **Confirm a fix site by + changing it and watching the symptom move**, not by name. + +- **Payoff, as predicted by the entry:** both blocked example files are now + deterministic and have been promoted to FORMAL — + `examples/expectations/stats_examples.json` and + `statistical_analysis.json`, each verified stable over five consecutive runs. + +- **Spun off:** [P42](#p42) (`MODE` is numeric-only, which is why the corpus + cases here are all numeric — the string form is where the divergence is easiest + to see and is exactly what we cannot yet express) and [P43](#p43) (`RANGE` + endpoint semantics, found because the repro query above is a 25/25 tie for us + and would not be for DuckDB). + +--- + +### P42 — `MODE` rejects non-numeric values, and returns a float for integers +- **Status:** 🔴 OPEN — hard error, loudly signposted +- **Corpus:** none yet. Deliberately: adding one means pinning a `GAP`, and the + decision below should be taken first. +- **Observed:** `SELECT MODE(region) FROM international_sales` → + *"MODE currently only supports numeric values"* + (`aggregate_functions/mod.rs`, `CollectorState::accumulate`). DuckDB returns + `'Europe'`. The live implementation collects into a `Vec`, so any + non-numeric input is an error by construction, and the result is always + `DataValue::Float` — `MODE` over an integer column returns `50.0`, not `50`. +- **Worse in the grouped form.** `SELECT region, MODE(product) FROM + international_sales GROUP BY region` does not error — it returns one row per + region with an **empty** `MODE` column. So the same defect is loud in one shape + and silent in the other, which is the [R3](ENGINE_REFACTORING.md#r3) pattern. +- **Decision:** **Fix**, and note that the shadowed `ModeState` in + `src/sql/aggregates/mod.rs` already does the right thing — it keys on a string + rendering and returns the *original* `DataValue`, so it handles strings, dates + and booleans and preserves type. The fix is most likely to make the live path + do what the dead one already does, which makes this a natural companion to + [R12](ENGINE_REFACTORING.md#r12) rather than an independent piece of work. +- **Why it matters beyond the error message:** MODE of a *category* is the common + use ("most frequent product"), far more so than MODE of a measure. And it + costs the corpus its best test: the discriminating tie-break case is much + clearer on strings (`'delta'` vs `'alpha'`) than on the integer column + `mode_tie_first_seen_not_smallest` had to fall back to. + +--- + +### P43 — `RANGE(a, b)` is inclusive of `b`; DuckDB's is half-open +- **Status:** 🔴 OPEN — silent, and it changes row counts +- **Corpus:** none yet — `RANGE` appears nowhere in the corpus, which is how + this went unnoticed. +- **Observed:** `SELECT COUNT(*), MIN(value), MAX(value) FROM RANGE(1,50)` gives + us `50, 1, 50`. DuckDB's `range(1,50)` yields **49** rows, `1`–`49`: the stop + bound is exclusive, matching Python's `range` and DuckDB's own documentation. + `generate_series` is the inclusive spelling there. +- **Found:** 2026-09-06, while building the [P41](#p41) corpus cases. The P41 + repro relies on `RANGE(1,50)` being a perfect 25/25 even/odd split — true for + us, false for the reference (25 odd, 24 even), so the query could not be + lifted into the corpus as written. +- **Decision:** **Not yet taken.** Unlike most entries here this is not obviously + a fix: changing it is a breaking change for every existing query and example + that uses `RANGE`, and the off-by-one lands silently in each. Sequence it as a + decision — match the reference and sweep the examples, or diverge deliberately + and record it under *Deferred* — not as a quiet correction. Adding + `GENERATE_SERIES` as the inclusive spelling is the move that makes matching + the reference survivable. +- **Related:** the same class as [P36](#p36) and [P39](#p39) — a defensible local + choice that only becomes a problem because it is undocumented and unpinned. --- diff --git a/examples/expectations/statistical_analysis.json b/examples/expectations/statistical_analysis.json new file mode 100644 index 0000000..3d5ed88 --- /dev/null +++ b/examples/expectations/statistical_analysis.json @@ -0,0 +1,149 @@ +[ + [ + { + "average_sales_amount": 18291.666666666668, + "median_sales_amount": 18000.0, + "most_common_sales_amount": 15000.0, + "stddev_pop_explicit": 4568.726725126913, + "stddev_population": 4666.990154419578, + "stddev_sample": 4666.990154419572, + "total_records": 24, + "variance_pop_explicit": 20873263.888888836, + "variance_population": 21780797.101449277, + "variance_sample": 21780797.101449218 + } + ], + [ + { + "avg_sales_amount": 22000.0, + "max_sales_amount": 28000, + "median_sales_amount": 22000.0, + "min_sales_amount": 16000, + "region": "West", + "sales_count": 6, + "stddev": 4732.86, + "variance": 22400000.0 + }, + { + "avg_sales_amount": 20833.33, + "max_sales_amount": 25000, + "median_sales_amount": 20500.0, + "min_sales_amount": 17000, + "region": "South", + "sales_count": 6, + "stddev": 2857.74, + "variance": 8166666.67 + }, + { + "avg_sales_amount": 16166.67, + "max_sales_amount": 22000, + "median_sales_amount": 15500.0, + "min_sales_amount": 12000, + "region": "North", + "sales_count": 6, + "stddev": 3488.07, + "variance": 12166666.67 + }, + { + "avg_sales_amount": 14166.67, + "max_sales_amount": 19000, + "median_sales_amount": 13500.0, + "min_sales_amount": 11000, + "region": "East", + "sales_count": 6, + "stddev": 2714.16, + "variance": 7366666.67 + } + ], + [ + { + "mean": 17615.38, + "product": "Widget", + "sample_size": 13, + "stddev_difference": 0.0, + "stddev_pop": 4941.97, + "stddev_samp": 4941.97, + "var_pop": 24423076.92, + "var_samp": 24423076.92, + "variance_difference": 0.0 + }, + { + "mean": 19090.91, + "product": "Gadget", + "sample_size": 11, + "stddev_difference": 0.0, + "stddev_pop": 4414.85, + "stddev_samp": 4414.85, + "var_pop": 19490909.09, + "var_samp": 19490909.09, + "variance_difference": 0.0 + } + ], + [ + { + "most_common_hundred_range": 15000.0, + "most_common_sales_amount": 15000.0, + "most_common_thousand_range": 15.0 + } + ], + [ + { + "most_common_region": "North", + "occurrences": 6 + } + ], + [ + { + "most_common_product": "Widget", + "occurrences": 13 + } + ], + [ + { + "coefficient_of_variation": 28.05, + "mean": 17615.38, + "n": 13, + "product": "Widget", + "std_dev": 4941.97 + }, + { + "coefficient_of_variation": 23.13, + "mean": 19090.91, + "n": 11, + "product": "Gadget", + "std_dev": 4414.85 + } + ], + [ + { + "avg_sales_amount": 16000.0, + "max_sales_amount": 24000, + "median_sales_amount": 15500.0, + "min_sales_amount": 11000, + "month": "2024-01", + "range": 13000, + "transactions": 8, + "volatility": 4342.48 + }, + { + "avg_sales_amount": 18000.0, + "max_sales_amount": 26000, + "median_sales_amount": 18000.0, + "min_sales_amount": 13000, + "month": "2024-02", + "range": 13000, + "transactions": 8, + "volatility": 4208.83 + }, + { + "avg_sales_amount": 20875.0, + "max_sales_amount": 28000, + "median_sales_amount": 21000.0, + "min_sales_amount": 14000, + "month": "2024-03", + "range": 14000, + "transactions": 8, + "volatility": 4611.71 + } + ] +] \ No newline at end of file diff --git a/examples/expectations/stats_examples.json b/examples/expectations/stats_examples.json new file mode 100644 index 0000000..e322bb6 --- /dev/null +++ b/examples/expectations/stats_examples.json @@ -0,0 +1,154 @@ +[ + [ + { + "analysis": "AAPL Closing Prices", + "maximum": 179.26, + "mean": 109.06669849086583, + "median": 109.01, + "minimum": 55.7899, + "observations": 1259, + "p50_percentile": 109.01, + "std_deviation": 30.556811676964685, + "variance": 933.7187398614853 + } + ], + [ + { + "analysis": "AAPL Volume Analysis", + "avg_volume": 54047899.73550437, + "max_volume": 266833581, + "median_volume": 45668931.0, + "min_volume": 11475922, + "trading_days": 1259, + "volume_volatility": 33468353.335784018 + } + ], + [ + { + "analysis": "Price Range Distribution", + "avg_daily_range": 1.8095294678316098, + "avg_range_percent": 1.708748072581743, + "max_daily_range": 16.799999999999997, + "median_range": 1.5300000000000011, + "min_daily_range": 0.45999999999999375, + "range_volatility": 1.1173697868377992, + "total_days": 1259 + } + ], + [ + { + "avg_price_in_bucket": 134.74261705685606, + "frequency": 598, + "max_price_in_bucket": 179.26, + "min_price_in_bucket": 110.06, + "percentage": 47.49801429706116, + "price_bucket": "Over $110" + }, + { + "avg_price_in_bucket": 95.84045443786984, + "frequency": 169, + "max_price_in_bucket": 99.99, + "min_price_in_bucket": 90.28, + "percentage": 13.423351866560763, + "price_bucket": "$90-100" + }, + { + "avg_price_in_bucket": 105.87450609756102, + "frequency": 164, + "max_price_in_bucket": 109.99, + "min_price_in_bucket": 100.11, + "percentage": 13.026211278792692, + "price_bucket": "$100-110" + }, + { + "avg_price_in_bucket": 63.41907058823527, + "frequency": 153, + "max_price_in_bucket": 69.9482, + "min_price_in_bucket": 55.7899, + "percentage": 12.152501985702939, + "price_bucket": "Under $70" + }, + { + "avg_price_in_bucket": 75.1279766423358, + "frequency": 137, + "max_price_in_bucket": 79.6428, + "min_price_in_bucket": 70.0914, + "percentage": 10.881652104845115, + "price_bucket": "$70-80" + }, + { + "avg_price_in_bucket": 83.75742105263156, + "frequency": 38, + "max_price_in_bucket": 89.8071, + "min_price_in_bucket": 80.0028, + "percentage": 3.0182684670373314, + "price_bucket": "$80-90" + } + ], + [ + { + "analysis": "Synthetic Data (1-20)", + "max_val": 20, + "mean": 10.5, + "median": 10.5, + "min_val": 1, + "mode_value": 1.0, + "n": 20, + "std_dev": 5.916079783099616, + "variance": 35.0 + } + ], + [ + { + "analysis": "Most Frequent Price Levels", + "avg_rounded_price": 109.08657664813344, + "median_rounded_price": 109.0, + "most_frequent_price": 109.0, + "total_observations": 1259 + } + ], + [ + { + "avg_change": 0.44312450738916304, + "frequency": 812, + "max_change": 0.9984999999999999, + "min_change": 0.0, + "percentage": 64.49563145353456, + "volatility_class": "Low Volatility", + "volatility_stddev": 0.28721884296002986 + }, + { + "avg_change": 1.607333740831296, + "frequency": 409, + "max_change": 2.989999999999995, + "min_change": 1.0, + "percentage": 32.48610007942812, + "volatility_class": "Medium Volatility", + "volatility_stddev": 0.5312862891282142 + }, + { + "avg_change": 3.994642105263159, + "frequency": 38, + "max_change": 8.25, + "min_change": 3.030000000000001, + "percentage": 3.0182684670373314, + "volatility_class": "High Volatility", + "volatility_stddev": 1.3585912625656669 + } + ], + [ + { + "arithmetic_mean": 25.5, + "dataset": "Range Analysis (1-50)", + "maximum": 50, + "mean_quadratic": 858.5, + "median_quadratic": 650.5, + "median_value": 25.5, + "minimum": 1, + "modal_parity": "Odd", + "n": 50, + "standard_deviation": 14.577379737113251, + "variance": 212.5 + } + ] +] \ No newline at end of file diff --git a/src/sql/aggregate_functions/mod.rs b/src/sql/aggregate_functions/mod.rs index eedbbd0..a1c4533 100644 --- a/src/sql/aggregate_functions/mod.rs +++ b/src/sql/aggregate_functions/mod.rs @@ -823,16 +823,27 @@ impl AggregateState for CollectorState { } } CollectorFunction::Mode => { + // Tally each distinct value, remembering the position it was + // first seen at. `values` is in input order, so that position + // is the tie-break: highest count wins, and on a tie the value + // seen earliest wins. + // + // Without the tie-break the winner came from `HashMap` + // iteration order, so the same binary on the same data returned + // different answers run to run (P41). Earliest-seen is the + // reference engine's rule. use std::collections::HashMap; - let mut counts = HashMap::new(); - for value in &self.values { - *counts.entry(value.to_bits()).or_insert(0) += 1; - } - if let Some((bits, _)) = counts.iter().max_by_key(|&(_, count)| count) { - DataValue::Float(f64::from_bits(*bits)) - } else { - DataValue::Null + let mut counts: HashMap = HashMap::new(); + for (position, value) in self.values.iter().enumerate() { + let entry = counts.entry(value.to_bits()).or_insert((0, position)); + entry.0 += 1; } + counts + .into_iter() + .max_by(|(_, a), (_, b)| a.0.cmp(&b.0).then_with(|| b.1.cmp(&a.1))) + .map_or(DataValue::Null, |(bits, _)| { + DataValue::Float(f64::from_bits(bits)) + }) } CollectorFunction::StdDev | CollectorFunction::Variance => { // Sample standard deviation and variance @@ -990,3 +1001,79 @@ mod tests { ); } } + +#[cfg(test)] +mod mode_tie_break_tests { + use super::{AggregateFunctionRegistry, DataValue}; + + /// Drives MODE through the registry, which is what the evaluator does — the + /// point of these tests is the path that actually runs, not a state type + /// that happens to be named after the function. See P41 in SQL_PARITY.md. + fn mode(values: &[DataValue]) -> DataValue { + let registry = AggregateFunctionRegistry::new(); + let func = registry.get("MODE").expect("MODE should be registered"); + let mut state = func.create_state(); + for v in values { + state + .accumulate(v) + .expect("MODE accumulate should not fail"); + } + state.finalize() + } + + fn ints(values: &[i64]) -> Vec { + values.iter().map(|i| DataValue::Integer(*i)).collect() + } + + #[test] + fn outright_winner_is_the_most_frequent_value() { + assert_eq!(mode(&ints(&[7, 3, 7, 3, 7])), DataValue::Float(7.0)); + } + + #[test] + fn a_tie_resolves_to_the_value_seen_first() { + assert_eq!(mode(&ints(&[0, 0, 1, 1])), DataValue::Float(0.0)); + assert_eq!(mode(&ints(&[1, 1, 0, 0])), DataValue::Float(1.0)); + } + + #[test] + fn the_tie_break_is_first_seen_not_smallest() { + // The discriminating case: every value ties at one occurrence and the + // first one seen is not the smallest. "Smallest wins" would pass the + // test above and still diverge from the reference engine here. + assert_eq!(mode(&ints(&[5, 3, 9, 1])), DataValue::Float(5.0)); + } + + #[test] + fn nulls_are_ignored_and_do_not_shift_the_tie_break() { + let values = vec![ + DataValue::Null, + DataValue::Integer(9), + DataValue::Null, + DataValue::Null, + DataValue::Integer(2), + DataValue::Null, + ]; + assert_eq!(mode(&values), DataValue::Float(9.0)); + } + + #[test] + fn all_null_and_empty_inputs_are_null() { + assert_eq!(mode(&[]), DataValue::Null); + assert_eq!(mode(&[DataValue::Null, DataValue::Null]), DataValue::Null); + } + + #[test] + fn repeated_runs_over_a_tie_give_the_same_answer() { + // The P41 symptom itself: the old implementation took whichever entry + // `HashMap` iteration surfaced last, so a perfect tie returned a + // different value between runs of the same binary. Enough distinct keys + // that hash ordering actually varies. + let values: Vec = (0..64).map(|i| DataValue::Integer(i % 32)).collect(); + let first = mode(&values); + assert_eq!(first, DataValue::Float(0.0)); + for _ in 0..50 { + assert_eq!(mode(&values), first); + } + } +} diff --git a/src/sql/aggregates/mod.rs b/src/sql/aggregates/mod.rs index b2e294d..c26a0c2 100644 --- a/src/sql/aggregates/mod.rs +++ b/src/sql/aggregates/mod.rs @@ -409,10 +409,22 @@ impl PercentileState { } } +/// One distinct value's tally for MODE, carrying the position of its first +/// occurrence so that ties are broken by input order rather than by hash +/// iteration order. +#[derive(Debug, Clone)] +pub struct ModeTally { + pub value: DataValue, + pub count: i64, + pub first_seen: u64, +} + /// State for MODE aggregation (most frequent value) #[derive(Debug, Clone)] pub struct ModeState { - pub counts: std::collections::HashMap, + pub counts: std::collections::HashMap, + /// Position of the next non-NULL value, used only for tie-breaking. + next_position: u64, } impl Default for ModeState { @@ -426,6 +438,7 @@ impl ModeState { pub fn new() -> Self { Self { counts: std::collections::HashMap::new(), + next_position: 0, } } @@ -449,26 +462,35 @@ impl ModeState { DataValue::Null => return Ok(()), }; - // Update count and store the original value - let entry = self.counts.entry(key).or_insert((value.clone(), 0)); - entry.1 += 1; + // Update count and store the original value, remembering where the + // value was first seen so ties resolve deterministically. + let position = self.next_position; + self.next_position += 1; + let entry = self.counts.entry(key).or_insert_with(|| ModeTally { + value: value.clone(), + count: 0, + first_seen: position, + }); + entry.count += 1; Ok(()) } + /// Highest count wins; on a tie the value seen earliest in the input wins. + /// + /// The tie-break matters: without it the winner came from `HashMap` + /// iteration order, so the same binary on the same data returned different + /// answers run to run (P41). Earliest-seen is the reference engine's rule. #[must_use] pub fn finalize(self) -> DataValue { - if self.counts.is_empty() { - return DataValue::Null; - } - - // Find the value with the highest count - let max_entry = self.counts.iter().max_by_key(|(_, (_, count))| count); - - match max_entry { - Some((_, (value, _count))) => value.clone(), - None => DataValue::Null, - } + self.counts + .into_values() + .max_by(|a, b| { + a.count + .cmp(&b.count) + .then_with(|| b.first_seen.cmp(&a.first_seen)) + }) + .map_or(DataValue::Null, |tally| tally.value) } } @@ -717,3 +739,84 @@ pub fn is_constant_expression(expr: &crate::recursive_parser::SqlExpression) -> pub fn is_aggregate_compatible(expr: &crate::recursive_parser::SqlExpression) -> bool { contains_aggregate(expr) || is_constant_expression(expr) } + +#[cfg(test)] +mod mode_tie_break_tests { + use super::{DataValue, ModeState}; + + fn mode(values: &[DataValue]) -> DataValue { + let mut state = ModeState::new(); + for v in values { + state.add(v).expect("MODE add should not fail"); + } + state.finalize() + } + + fn ints(values: &[i64]) -> Vec { + values.iter().map(|i| DataValue::Integer(*i)).collect() + } + + fn strings(values: &[&str]) -> Vec { + values + .iter() + .map(|s| DataValue::String((*s).to_string())) + .collect() + } + + #[test] + fn outright_winner_is_the_most_frequent_value() { + assert_eq!(mode(&ints(&[7, 3, 7, 3, 7])), DataValue::Integer(7)); + } + + #[test] + fn a_tie_resolves_to_the_value_seen_first() { + assert_eq!(mode(&ints(&[0, 0, 1, 1])), DataValue::Integer(0)); + assert_eq!(mode(&ints(&[1, 1, 0, 0])), DataValue::Integer(1)); + } + + #[test] + fn the_tie_break_is_first_seen_not_smallest() { + // The discriminating case: every value ties at one occurrence, and the + // first one seen is not the smallest. Picking the smallest would pass + // the test above and still diverge from the reference engine here. + assert_eq!(mode(&ints(&[5, 3, 9, 1])), DataValue::Integer(5)); + assert_eq!( + mode(&strings(&["delta", "charlie", "bravo", "alpha"])), + DataValue::String("delta".to_string()) + ); + } + + #[test] + fn nulls_are_ignored_and_do_not_shift_the_tie_break() { + // NULLs never win, and interleaving them must not reorder the survivors. + let values = vec![ + DataValue::Null, + DataValue::Integer(9), + DataValue::Null, + DataValue::Null, + DataValue::Integer(2), + DataValue::Null, + ]; + assert_eq!(mode(&values), DataValue::Integer(9)); + } + + #[test] + fn all_null_and_empty_inputs_are_null() { + assert_eq!(mode(&[]), DataValue::Null); + assert_eq!(mode(&[DataValue::Null, DataValue::Null]), DataValue::Null); + } + + #[test] + fn repeated_runs_over_a_tie_give_the_same_answer() { + // This is the P41 symptom itself: the old implementation took whichever + // entry `HashMap` iteration surfaced last, so a perfect tie returned a + // different value between runs of the same binary. Enough distinct keys + // to make hash ordering actually vary. + let values: Vec = (0..64).map(|i| DataValue::Integer(i % 32)).collect(); + let first = mode(&values); + assert_eq!(first, DataValue::Integer(0)); + for _ in 0..50 { + assert_eq!(mode(&values), first); + } + } +} diff --git a/tests/comparison/corpus/10_aggregate_nulls.toml b/tests/comparison/corpus/10_aggregate_nulls.toml index 1834d57..7cec14c 100644 --- a/tests/comparison/corpus/10_aggregate_nulls.toml +++ b/tests/comparison/corpus/10_aggregate_nulls.toml @@ -158,3 +158,50 @@ expect = "DIFFER" # this one is arguably a coercion-first design choice rather than a bug — it # needs a decision recorded, not an automatic fix. + +# --------------------------------------------------------------------------- +# P41 — MODE tie-breaking. Added 2026-09-06 with the fix. +# +# MODE had no tie-break rule at all: it took whichever entry `HashMap` +# iteration surfaced last, so a tie returned a different value between runs of +# the same binary. These cases pin the rule the reference engine uses — +# earliest-seen value wins — which is only observable on a tie, so the corpus +# needs a tie where the answer is NOT the value a plausible alternative rule +# would pick. +# +# MODE is numeric-only for us (see P42), so every case here uses a numeric +# column. The string form is where the divergence is easiest to see, and it is +# exactly what we cannot yet express. +# --------------------------------------------------------------------------- + +[[case]] +id = "mode_two_way_tie" +data = "null_edges.csv" +sql = "SELECT MODE(score) AS m FROM null_edges" +# 50 appears twice and 70 appears twice — a two-way tie, won by 50 on +# first-seen. This is also the case where first-seen and "smallest wins" happen +# to agree, which is exactly why it cannot stand alone. + +[[case]] +id = "mode_tie_first_seen_not_smallest" +data = "null_edges.csv" +sql = "SELECT MODE(partner_id) AS m FROM null_edges" +# The discriminating case. Every non-NULL partner_id is distinct +# (3, 1, 5, 4, 8, 7, 11), so all seven tie at one occurrence. First-seen gives +# 3; "smallest value wins" — the rule P41 was originally going to adopt — gives +# 1. DuckDB gives 3. Without this case the corpus cannot tell the two apart. + +[[case]] +id = "mode_all_null" +data = "null_edges.csv" +sql = "SELECT MODE(bonus) AS m FROM null_edges" +# Every bonus is NULL. NULLs are ignored, so there is nothing to tally and the +# answer is NULL rather than an error or a missing row. + +[[case]] +id = "mode_grouped" +data = "null_edges.csv" +sql = "SELECT team, MODE(score) AS m FROM null_edges GROUP BY team ORDER BY team" +# Per-group state: each group must tally independently, and the all-NULL group +# ('delta') must still produce its row with NULL. Catches a fix applied to the +# ungrouped path only.