Result-set comparator - #239
Open
vishalkalbi27 wants to merge 4 commits into
Open
Conversation
…an be compared
Raw cells cannot be compared. True == 1 == 1.0 == Decimal(1) and all four hash
alike, so a Counter reads a boolean column as an integer column of zeros and
ones; float('nan') != itself yet dict's identity fast-path collapses the same
NaN object anyway, making a row count depend on driver object reuse; and
Decimal('0.1') != 0.1, so one number read through two drivers disagrees.
Every cell becomes a hashable (type_tag, value) key instead. Numbers normalise
through an explicit decimal context (the ambient one is process-global and any
caller can narrow it), a float goes through Decimal(repr(x)) rather than
Decimal(x), dates canonicalise to ISO text with an aware value read as an
instant in UTC, and text is left exactly as it came — folding it would hide the
difference a comparator exists to find.
Only the canonicalisation layer; comparison and scoring sit on top of it later.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… they share Three steps on top of the canonical cell keys, kept separate because each is wrong in its own way. Is the result ordered? Read off the top-level node's own `order` argument and never with a search for an Order anywhere in the tree: a subquery's ORDER BY, a CTE's, an OVER (ORDER BY ...) and an array_agg(x ORDER BY x) all order something other than the result, and a search finds every one of them. The union cases come out right in both directions for free — an ORDER BY after a UNION hangs off the Union node and is a total order, one inside a single arm is not. The dialect is threaded through because a generic parse of a backtick-quoting statement raises rather than merely losing detail. A statement that is missing, empty or unparseable is read as ORDERED: the permissive reading would silently stop checking an ordering the author asked for, and a visible false failure is recoverable where a silent weakening is not. Which column answers which? By VALUES only — never by name, never by position. A generated statement that aliases the total and selects it second still answered the question, and one that reused the golden name for a different value did not. Pairing is an augmenting-path matching so that columns carrying identical values cannot collapse onto one partner. How far do the rows agree? As a multiset unless the author ordered them: a row returned twice where the answer key has it once is a different answer, usually a join that fanned out, and a set comparison is exactly the one that hides it. A row narrower than its column list — which ExecResult never validates — is raised as this module's own RaggedRow, so the caller can report the case rather than field an IndexError from a projection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`compare_result_sets` is the way in: it reads whether the answer key ordered its rows, dispatches to the level the author asked for, and hands back a frozen `ItemScore`. It is total — a ragged row, a malformed result, or a band that cannot be applied come back with an error status rather than as an exception, so one bad item costs that item and not the run. The score carries verdicts, row counts and column names. Never a cell and never the answer key's SQL: the payload being judged here is result data, and a score travels further than the run that produced it. Two empty results are UNSCORED rather than a pass — they agree about nothing, and calling that a full match is how a statement returning nothing gates like a right one. And accuracy is capped just short of the 1.0 pass mark unless the overlap is complete: rounding to three places alone would hand a pass to 4002 of 4004 rows. Also replaces the augmenting-path column matching with a greedy scan. Columns pair on EQUALITY of their value vectors, and equality is transitive, so the candidates are equivalence classes and partners inside one are interchangeable — there is no augmenting path to find. The matching tests are unchanged and still pass.
A review of the scoring call found six places where it reported a verdict weaker, or plainer wrong, than the one it had actually reached: * A top-level ORDER BY was read off `args['order']` of whatever node parsed out. A trailing comment, a leading SET, a parenthesised statement or an EXPLAIN all parse to nodes that carry no such argument, so the answer key's ordering was silently dropped and the item scored a full 1.0 on reversed rows with no note. A parenthesised statement is now unwrapped, and a root that cannot carry an order takes the assumed-ordered path the module's own comment demands. * An unknown dialect raises ValueError and a deeply nested statement a RecursionError, neither of them a SqlglotError, and that read happens outside the totality net — so a call documented as never raising did. * The both-empty guard ran before the level dispatch, which dropped exactly the failure `nonempty` and `bounded` exist to catch: those levels have no answer key by design, so their golden side is legitimately empty. * `values` rounded whole numbers to nine significant digits, so any id or count above ~1e9 compared equal to its neighbours. Whole numbers carry no floating-point tail to forgive and are no longer bucketed; the comment and docstring now say bucket rather than tolerance, which is what it is. * A row-count difference was laundered through the unmatched-column branch and reported as "no generated column carries the values of: id" — the string a person reads for the most common regression there is. It is checked before any column is paired now, and names rows. * A band with both a row half and a value half could never reach the row half: a result the value band could not be applied to short-circuited as an error, leaving the item permanently unjudgeable. It falls through to the row band, and an empty result scores 0.0 rather than erroring. Also: the accuracy is the raw share rather than a rounded-then-capped one (three decimals belong to the report renderer), `__all__` names only the two symbols that are the interface, and the value band's inclusive edges are pinned by tests that a `<`/`<=` mutation now fails. The tests' zero-padded identifier fixture was a real deployment's code carried over from the comparator this ports; it is synthetic now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishalkalbi27
requested review from
ashwin-agami and
sandeep-agami
as code owners
August 25, 2026 12:15
There was a problem hiding this comment.
Pull request overview
Adds a deterministic, pure-Python result-set comparator to score golden-eval items by comparing an answer key’s result set to a generated SQL result set at configurable strictness levels. This lives in the semantic_model layer and is exercised by an extensive new test suite.
Changes:
- Introduces
semantic_model/comparator.pywithcompare_result_sets(...) -> ItemScore, including cell canonicalization, top-levelORDER BYdetection, value-based column matching, and scoring across five match levels. - Adds
tests/test_ah099_comparator.pyto pin canonicalization behavior (bool/num/NaN/date), ordering detection, column pairing semantics, and end-to-end scoring behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/agami-core/src/semantic_model/comparator.py | Implements the result-set comparator and scoring API used for deterministic golden evaluation. |
| tests/test_ah099_comparator.py | Adds comprehensive unit tests validating canonicalization, ordering detection, column matching, and scoring rules. |
Suppressed comments (2)
packages/agami-core/src/semantic_model/comparator.py:514
- The row-count mismatch reason string is missing a verb ("the generated result 3"), which makes diagnostics harder to read. Include "has" in the message.
return _Verdict(
"scored", 0.0,
f"the answer key has {len(golden.rows)} rows and the generated result "
f"{len(generated.rows)}",
)
packages/agami-core/src/semantic_model/comparator.py:520
- The column-count mismatch reason string is missing a verb ("the generated result 3"), which makes diagnostics harder to read. Include "has" in the message.
return _Verdict(
"scored", 0.0,
f"the answer key has {len(golden.columns)} columns and the generated result "
f"{len(generated.columns)}",
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # test is on the VALUE and not on the Python type, because the same id arrives as an int from | ||
| # one driver and a Decimal from another, and exempting only one of them would fail two | ||
| # identical numbers. | ||
| if quantize and dec != dec.to_integral_value(): |
Comment on lines
+458
to
+462
| return _Verdict( | ||
| "scored", 0.0, | ||
| f"the answer key has {len(golden.rows)} rows and the generated result " | ||
| f"{len(generated.rows)}", | ||
| ) |
Comment on lines
+485
to
+489
| return _Verdict( | ||
| "scored", accuracy, | ||
| f"{overlap} of the answer key's {golden_count} rows matched, " | ||
| f"out of {generated_count} the generated statement returned", | ||
| ) |
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.
Spec: AH-099
Summary
Decide whether two SQL statements returned the same answer, in code, at the strictness the dataset
author asked for. Given the answer key's result set and the one the agent's generated SQL produced,
return an
ItemScore. This is what makes golden scoring deterministic execution accuracy ratherthan a model's opinion — it is the reason the eval is reproducible.
Pure function. It executes nothing, writes nothing, opens no connection, and has no way to make a
model call at all.
Changes
packages/agami-core/src/semantic_model/comparator.py(new) — one public entry point,compare_result_sets(golden, generated, *, match, golden_sql, bounds, dialect) -> ItemScore,over four layers: cell canonicalization, top-level
ORDER BYdetection, column matching, and thefive
matchlevels.MatchLevelandGoldenBoundsare imported fromgolden.py, neverredefined;
ExecResultcomes fromexecute_sql.tests/test_ah099_comparator.py(new) — 190 tests.Three properties are load-bearing, and each is pinned by tests:
Columns match on their values — never on name, never on position. A correct statement that
renames and reorders its projection is still correct, and that is the common case a name-matched
comparator fails.
Cells canonicalize to a
(type_tag, value)key. Not stylistic: Python collapsesTrue,1,1.0andDecimal(1)into a singleCounterbucket, so without the tag a boolean column wouldcompare equal to an integer column of 0s and 1s. NaN gets a sentinel for the same reason — two
distinct NaN objects bucket separately while the same object collapses, so row counts would
otherwise depend on driver object identity.
Ordering is read from the golden statement alone, parsed rather than regex-matched, so a
generated statement that drops the
ORDER BYstill scores order-sensitively.ORDER BYinside asubquery, a CTE, an
OVER (…)or an aggregate does not make a result ordered.Decisions made during the build
Recorded in the spec's
## Decisions; the spec's only[NEEDS DECISION](tolerance) is resolvedin place.
not as a tolerance. Rows compare as a multiset, which needs hashable exactly-equal keys, and a
tolerance is not an equivalence relation. Whole numbers skip the bucket so an identifier or a
count is never merged with its neighbour. Two values straddling a bucket edge compare unequal
however close they are; that is stated rather than hidden.
matchlevel already encodes how loose thecomparison is, so a second threshold would let two places disagree about one verdict.
unscoredanderrorcarryaccuracy=None, never0.0— zero is a score an item canlegitimately earn.
boundedgained an authoredboundsfield, added to Golden dataset reader #237 while it was open. As shipped,match: boundedwas writable and its band was not, so one of the contract's five levels couldnever do anything.
equality is transitive, so candidate sets are equivalence classes and partners within one are
interchangeable — there is no augmenting path to find. The spec originally asserted the
opposite; the build disproved it and the code carries the argument in a comment so it is not
"fixed" back.
notes. Thepermissive reading would silently stop checking an ordering the author asked for.
Findings from review
Two review passes ran over the diff. Eight must-fixes, all reproduced, all fixed and re-verified.
Two were serious:
ORDER BYcould vanish silently. sqlglot wraps some statements in nodes carryingno
orderarg, soSELECT n FROM t ORDER BY n;followed by a trailing comment scoredorder-insensitively with no note — a full 1.0 on a result the answer key says is wrong. Same
for
EXPLAIN …,SET …; SELECT …, and a parenthesized statement. Now the root node is checkedand anything that cannot carry an order falls through to assumed-ordered plus a note.
nonemptyandbounded. Those levels have no answer key bydesign, so their golden side is legitimately empty — and an agent returning no rows, the
failure they exist to catch, was scored
unscoredand dropped from the run instead of 0.0. Theguard now applies only to the levels that consult the answer key.
Also fixed:
valuesquantization was up to 10× wider than documented and passed genuinelydifferent identifiers above ~1e9; a differing row count was reported as a missing column,
which corrupted the field the run report reads and would have told a reader a present column was
absent;
compare_result_setscould raise on an unknown dialect or deeply nested parentheses,falsifying the totality the caller is told to rely on; and a
boundeditem carrying both a row bandand a value band could never reach its row band.
Three defects were caught before review, during the build, and are worth naming because each
would have been invisible in production:
Decimal.normalize()reads the process-global decimalcontext, so an unrelated caller mutating
getcontext().precwould have changed comparison keys;round(accuracy, 3)turned 4002 of 4004 matching rows into exactly1.0, converting a failure intoa pass at the only threshold that matters; and the augmenting-path matching in decision 5 was
unjustified complexity.
One data-hygiene fix worth flagging: a fixture used the literal
01100170109835, which is areal California Department of Education CDS code carried over from the comparator being ported —
not a synthetic identifier. It was replaced with a synthetic value here and scrubbed from the
spec, which is where it originated, so it cannot be reintroduced by the next slice. It never
reached a push.
Pre-existing, not fixed here:
GoldenBoundspermits a row band and a value band together withno cross-check (parent branch), and
ExecResultdoes not validate that a row is as wide as itscolumn list — the latter is handled defensively via
RaggedRow.Checklist
Spec: AH-099)E2E items and REQ-006)
ruff checkclean; gitleaks cleansubject (the
nonemptyfixture's fabricated golden; two accuracy assertions after roundingwas removed) — the regressions they guard are preserved.
shipped sample store
findings dispositioned
modelextrasemantic_model/__init__.pyuntouched;comparator.pycorrectly absent from the stdlib-onlyvendored mirror, and imported by no plugin script
🤖 Generated with Claude Code