fix(engine): give MODE a deterministic tie-break (P41) - #67
Merged
Conversation
MODE tallied into a HashMap and took the highest count with no tie-break
rule, so on a tie the winner was whichever entry hash iteration surfaced
last. The same binary on the same data returned different answers between
runs: six runs of the 25/25 parity tie gave `0 1 1 0 0 1`.
Break ties by first occurrence in the input, which is what DuckDB does
(mode.cpp tracks a first_row per distinct value and compares
`count > best.count || (count == best.count && first_row < best.first_row)`).
The entry had proposed "smallest value wins"; probing the reference showed
that is a different rule — ('b','b','a','a') gives 'b', and (5,3,9,1) gives
5, not 1 — so this follows the reference per the doc's own convention.
The finding named `ModeState` in src/sql/aggregates/mod.rs. Fixing it there
changed nothing: there are two aggregate registries and ArithmeticEvaluator
checks the newer one first, so the live MODE is CollectorState in
src/sql/aggregate_functions/mod.rs. Both are fixed here; the duplication is
filed as R12.
- Corpus: four new cases in tier 10, including one where first-seen and
"smallest wins" disagree, so the two rules are distinguishable. 177 -> 181
cases, 152 -> 156 AGREE, no other bucket moved.
- Rust regression tests in both modules, six cases each.
- examples/stats_examples.sql and statistical_analysis.sql promoted to
FORMAL — they were blocked on this exact nondeterminism.
- Spun off P42 (MODE is numeric-only and returns a float for integers) and
P43 (our RANGE stop bound is inclusive; DuckDB's is exclusive).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBEUeckCwmWXoWTpQUTDqP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MODE tallied into a HashMap and took the highest count with no tie-break rule, so on a tie the winner was whichever entry hash iteration surfaced last. The same binary on the same data returned different answers between runs: six runs of the 25/25 parity tie gave
0 1 1 0 0 1.Break ties by first occurrence in the input, which is what DuckDB does (mode.cpp tracks a first_row per distinct value and compares
count > best.count || (count == best.count && first_row < best.first_row)). The entry had proposed "smallest value wins"; probing the reference showed that is a different rule — ('b','b','a','a') gives 'b', and (5,3,9,1) gives 5, not 1 — so this follows the reference per the doc's own convention.The finding named
ModeStatein src/sql/aggregates/mod.rs. Fixing it there changed nothing: there are two aggregate registries and ArithmeticEvaluator checks the newer one first, so the live MODE is CollectorState in src/sql/aggregate_functions/mod.rs. Both are fixed here; the duplication is filed as R12.Claude-Session: https://claude.ai/code/session_01JBEUeckCwmWXoWTpQUTDqP