Make nine CrossDialectJoinTests row assertions order-independent (#332) - #337
Merged
Merged
Conversation
#332) Join_InnerJoin_OnClause, Join_WithWhere_OnLeftTable, Join_WithWhere_TwoCapturedParams_BooleanBetween_SequentialIndices and Where_BeforeJoin_GetsTableAliasQualification asserted (UserName, Total) rows positionally on PostgreSQL, MySQL and SQL Server from a join with no top-level ORDER BY. They passed only by accident of each planner's access path. Converts the pg/my/ss assertions to Is.EquivalentTo. The SQLite side stays positional -- its incidental insertion order is the deliberate reference shape the other three dialects mirror. No chain, no Prepare() and no expected-SQL string is touched, so the AssertDialects block these tests exist for is unchanged.
Join_InnerJoin_NamedTupleProjection and Join_ThreeTable_NamedTupleProjection indexed pg/my/ss results positionally on an unordered join. Both exist to prove named element access survives the join boundary, so the accessors are projected through .Select(r => (r.Name, r.Amount)) into Is.EquivalentTo rather than letting the names appear only in the expected literal -- this now exercises them on every row instead of only the ones the positional asserts reached. Join_ThreeTable_NamedTupleProjection previously asserted just [0] plus a Product-is-not-null check. No order-independent rendering of "row [0] is Alice/250.00" exists, since that row is not the ascending minimum over any projected column, so the real-provider sides now pin the full three-row multiset with real product names. Seeded order_items are one per order, so that set is fully determined.
…nt (#332) Select_Joined_Many_Sum_OnLeftTable, Select_Joined_Many_Count_OnLeftTable and Select_Joined_HasManyThrough_Max_OnLeftTable indexed pg/my/ss results positionally on an unordered join. These are the worst of the nine: the two Alice rows tie on the aggregate column as well (OrderTotal 325.50 for both, OrderCount 2 for both, MaxAddrId 2 for both), so the aggregate breaks no tie and there is no total order over the projection at all. Each converted block notes that inline. Completes the nine tests tracked in #332. The three remaining positional pg/my/ss sites in this file are correct -- each follows a SortedByAsync(r => r.UserName) over two rows with distinct usernames.
…ecedent The CrossDialectJoinTests <remarks> block declared these an open row-order flake and warned against fixing them. Rewrites it to explain why this file reaches for Is.EquivalentTo where the rest of the suite uses SortedByAsync -- the trap is still worth documenting so nobody simplifies it back to a sort. Avoids pinning a test count in the prose, since the pattern now covers more than the original nine. Updates llm-testing.md, which pointed at #332 as nine outstanding tests, to record the assertion-side remedy instead. Also aligns the nine conversions with Join_FiveTable_Select, which already sat in this file using a hoisted 'var expected = new[] { ... }' with Is.EquivalentTo and referencing #332 -- it is the established local pattern and the direct precedent for this work. Replaces the repeated inline arrays with one per test (net -46 lines). Kept per-test rather than a fixture-level static so expected values stay visible where they are read. Full suite green at 3501/3501, matching the pre-change baseline. No chain line and no expected-SQL string was touched on this branch, and ManifestOutput goldens are unchanged.
Addresses all six findings from the review pass (4 A, 2 B; no C or D).
F3 (M) -- the rewritten <remarks> claimed that for the navigation-aggregate
variants 'no total order over the projection exists at all'. That is false: the
three rows are pairwise distinct, so (UserName, Total) is a total order. The
real constraint is narrower -- no *ascending* key reproduces the originally
asserted sequence (250.00 before 75.50), and making a sort work would require
rewriting the expected sequence, which llm-testing.md forbids. Matters because
llm-testing.md now points readers here as the canonical explanation, and a
maintainer who inverts the false claim ('a total order does exist, so I may
sort') reintroduces the bug. The three inline aggregate comments carried the
same overstatement and are reworded.
F2 (L) -- Join_ThreeTable_NamedTupleProjection's SQLite side still asserted only
row [0] plus a Product-is-not-null check while pg/my/ss pinned the full 3-row
multiset, leaving the declared reference dialect weaker than its mirrors. It now
asserts all three rows with exact product names, still positionally.
F5 (L) -- <remarks> now names Join_WithWhere_OnRightTable,
Join_WithWhere_MultiParamAndBoolColumn_SequentialParamIndices and
Join_WithWhere_CapturedParam_OnRightTable as correct-as-sorts, so a future
row-order sweep does not convert them and lose genuine order coverage.
F4 (L) -- a comment claimed the replaced positional asserts reached 'only two'
rows; they reached all three. F1 (L) -- plan.md amended to record the hoisted
'expected' refactor. F6 (L) -- two ASCII '--' replaced with em dashes.
Full suite 3501/3501, matching baseline. Manifest goldens still unchanged.
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.
Summary
Nine tests in
CrossDialectJoinTestsasserted row values positionally on the PostgreSQL, MySQLand SQL Server sides of a
users → ordersjoin carrying no top-levelORDER BY. They passed onlyby accident of each planner's chosen access path — a statistics refresh, a parallel scan, or a hash
join picked instead of a nested loop could reorder the result set and turn them red with no code
change.
This converts the real-provider assertions to order-independent
Is.EquivalentTo. Every row andevery value stays asserted; only the accidental ordering assumption is dropped.
Reason for Change
The order these tests encoded was
orders.OrderIdascending (seed insertion order), butOrderIdis not in the projection, so it cannot be a client-side sort key — and the only projected
discriminator,
Total, runs descending within the Alice group. This is why the #314SortedByAsyncsweep, which now covers 118 sites, deliberately skipped these nine: aplausible-looking
(r.UserName, r.Total)key compiles, reads as correct, and silently swaps rows[0]and[1].State the trap precisely, because the imprecise version is dangerous:
(UserName, Total)is atotal order over these rows. It is just an ascending one, so it orders the two Alice rows opposite
to the asserted sequence. Sorting could only be made to work by rewriting the expected sequence to
fit the key — exactly what
llm-testing.mdforbids. The three navigation-aggregate variants offerno escape either: both Alice rows tie on the aggregate column as well (
OrderTotal325.50,OrderCount2,MaxAddrId2), so it contributes no discriminator beyondTotal.Impact
The pinned SQL is untouched. No chain, no
Prepare(), noSelect/Where/Join, and noexpected-SQL string was modified on this branch — verified mechanically across all branch commits.
The
AssertDialects(...)block is the bulk and the actual purpose of each of these tests, and it isbyte-identical after the change.
ManifestOutput/goldens are unchanged, confirming no chainregenerated.
The SQLite side stays positional throughout: its incidental insertion order is the deliberate
reference shape the other three dialects mirror, and keeping it positional preserves that signal.
Two files change:
src/Quarry.Tests/SqlOutput/CrossDialectJoinTests.csandllm-testing.md. Noproduction source, public API, analyzer, or generator is touched.
Plan items implemented as specified
(UserName, Total)Join_InnerJoin_OnClause,Join_WithWhere_OnLeftTable,Join_WithWhere_TwoCapturedParams_BooleanBetween_SequentialIndices,Where_BeforeJoin_GetsTableAliasQualificationJoin_InnerJoin_NamedTupleProjection,Join_ThreeTable_NamedTupleProjectionSelect_Joined_Many_Sum_OnLeftTable,Select_Joined_Many_Count_OnLeftTable,Select_Joined_HasManyThrough_Max_OnLeftTable<remarks>block rewritten;llm-testing.md:135updatedThe two named-tuple tests exist to prove named element access survives the join boundary, so their
accessors are projected through
.Select(r => (r.Name, r.Amount))intoIs.EquivalentToratherthan letting the names appear only in the expected literal — this exercises them on every row.
Deviations from plan implemented
Hoisted
expectedarrays. The plan specified the inlineIs.EquivalentTo(new[] { … })form.During step 4 it emerged that
Join_FiveTable_Select— already onmaster, in this same file —does exactly this conversion with a hoisted
var expected = new[] { … }and cites #332 in itscomment. That is the established local pattern and the direct precedent for this work, so all nine
were reworked to match (one
expectedper test, net −46 lines). Deliberately not hoisted to afixture-level static, so expected values stay visible where they are read.
Gaps in original plan implemented
Join_ThreeTable_NamedTupleProjectioncoverage. It asserted only row[0]plus aProduct is not nullcheck. There is no order-independent way to say "row[0]is Alice/250.00"— that row is not the ascending minimum over any projected column (75.50 < 250.00,
"Gadget" < "Widget"). All four dialects now pin the full three-row multiset with exact product
names. Seeded
order_itemsare one per order, so that set is fully determined. Strictly strongerthan what it replaced.
<remarks>names the exempt sites. Three tests in this file keepSortedByAsync(r => r.UserName)and are correct as-is — each returns two rows with distinct usernames, so the key is a total order
that reproduces the asserted sequence. The block now names them, so a future row-order sweep does
not convert them and lose genuine order coverage.
Migration Steps
None. Test-only change.
Performance Considerations
None.
Is.EquivalentToover three-element collections; no query, chain, or generated SQL changed.Security Considerations
None. No credential, connection-string, input-validation, or SQL-construction surface is touched.
Breaking Changes
None — consumer-facing or internal. No production code, public API, or generated output changes.
Verification
Quarry.Tests), plus 201/201 inQuarry.Migration.Tests. Docker available, nothingAssert.Ignored — the container-backeddialects really executed.
changed test count, as expected for an assertion-shape change.
Is.EquivalentTooverValueTuplewas confirmed empirically rather than assumed: temporarilychanging an expected
("Bob", 150.00m)to151.00mfailed with a preciseMissing (1)/Extra (1)diff. The converted assertions compare elements structurally.Review
Six findings, all addressed in
07f445a— 4 classified A, 2 classified B, none deferred ordismissed. The one worth calling out: the first
<remarks>rewrite claimed "no total order over theprojection exists at all" for the aggregate variants, which is false — the rows are pairwise
distinct. Since
llm-testing.mdnow directs readers to that block as the canonical explanation, amaintainer who checked the claim would either distrust the rest of the rationale or invert it ("a
total order does exist, so I may sort") and reintroduce the very bug the block exists to prevent.
Corrected to the accurate, narrower statement.