test(integ-test): add explicit ordering to order-dependent integration tests - #5723
Merged
mengweieric merged 7 commits intoAug 26, 2026
Merged
Conversation
Contributor
PR Reviewer Guide 🔍(Review updated until commit 1c85e22)Here are some key observations to aid the review process:
|
…tsIT
Seven tests run `head N` with no preceding sort and then assert exact values.
`head` over an unordered scan selects an undefined set of rows, so those values
hold only by accident of scan order, and a 5-shard index feeds `stats` a
different set:
testValuesFunctionGroupBy 1 shard: FURNITURE, OFFICE SUPPLIES
5 shards: OFFICE SUPPLIES, TECHNOLOGY
The engine is not at fault -- what differs is which rows reach `stats`, not how
they are aggregated.
`key` is unique in the CALCS fixture (17 distinct over 17 documents), so sorting
on it fixes the selection. Verified per test that 1-shard and 5-shard results
become identical and every existing expectation still holds, so no expected value
changes.
Four other `head` tests in this class are left alone: they assert only schema and
non-emptiness, so no exact value depends on the selection.
Signed-off-by: Eric Wei <menwe@amazon.com>
Five tests run `head 3` with no preceding sort and assert the values of the first three BANK documents. Sorting by account_number selects accounts 1, 6 and 13, whose balances (39225, 5686, 32838) and ages (32, 36, 28) are exactly what the tests already expect, so no expected value changes. Signed-off-by: Eric Wei <menwe@amazon.com>
testEvalStringConcatenationWithExistingData runs `head 3` with no preceding sort and asserts concatenated names from the first three BANK documents. Sorting by account_number selects accounts 1, 6 and 13, matching the existing expectations. Signed-off-by: Eric Wei <menwe@amazon.com>
…ionIT testMvappendWithFieldsAndLiterals runs `head 1` with no preceding sort and asserts age 32, which is the first BANK document only by accident of scan order. Sort by account_number, matching the neighbouring real-field tests in this class which already do so. Signed-off-by: Eric Wei <menwe@amazon.com>
Two tests run `head N` with no preceding sort and assert exact bins. testBinValueFieldOnly takes the first three time_test_data documents; @timestamp is unique across all 100, and the three earliest values (8945, 7623, 9187) give exactly the asserted bins. testBinSpanWithStartEndNeverShrinkRange takes six of the seven BANK documents; sorting by account_number yields ages 32, 36, 28, 33, 36, 39, matching the existing expectations. testBinTimestampSpan6Days and testBinTimestampSpan7Days are deliberately not fixed here. They already sort, but they sort after `bin` has replaced @timestamp with the bin label, so every row shares one key and the sort is not a total order. Correcting them needs either a pipeline reorder or new expected values, neither of which belongs in this change. Signed-off-by: Eric Wei <menwe@amazon.com>
Two tests compare a fully rendered table, row for row, against a query with no ORDER BY. Row order is unspecified, so the comparison holds only by accident of scan order and breaks once the index has more than one shard. account_number is unique in the fixture and ascends in the same order the tests already expect (1, 6, 13, 18, 20 -> Amber, Hattie, Nanette, Dale, Elinor), so ordering by it leaves the expected output byte for byte unchanged. All rows are retained. contentHeaderTest shares the query but asserts only the response content type, so it is left unordered. Signed-off-by: Eric Wei <menwe@amazon.com>
testSourceFieldQuery indexes two documents and compares the whole response body, including datarows, against a query with no sort. The expected order is hello then world, which holds only by accident of scan order. Sorting by name yields that same order, so the expected body is unchanged. Signed-off-by: Eric Wei <menwe@amazon.com>
mengweieric
force-pushed
the
fix/order-dependent-tests
branch
from
August 25, 2026 22:45
a2c4a6e to
1c85e22
Compare
Contributor
|
Persistent review updated to latest commit 1c85e22 |
mengweieric
marked this pull request as ready for review
August 25, 2026 22:56
mengweieric
requested review from
LantaoJin,
RyanL1997,
Swiddis,
acarbonetto,
ahkcs,
anirudha,
dai-chen,
joshuali925,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
August 25, 2026 22:56
RyanL1997
approved these changes
Aug 26, 2026
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.
Description
Seven integration-test classes assert exact values or exact output over a query whose row order was never defined. All are test-side: every changed file is under
src/test, no production code is touched, and no expected value changes — each fix adds an ordering that reproduces what the test already expects.Why one shard passes and multiple shards fail: with a single shard there is one segment order, so an unordered scan returns rows in a stable incidental order and an assertion depending on it passes by luck. With multiple shards the coordinator merges per-shard results, that incidental order changes, and the assertion breaks.
Why these are test defects and not engine bugs: in each case the engine returns correct data for whichever rows it was given — what differs is which rows reach the assertion, or in what order they are printed, neither of which the query constrained.
1.
CalciteMultiValueStatsIT—head Nwith no sortSeven tests run
head Nover an unordered scan and then assert exact values.headselects an undefined set of rows, so a 5-shard index feedsstatsa different set:Real failures:
Note
testValuesFunctionGroupBy: the group count differs, which normally suggests broken aggregation. It does not here — the input set differs, so a different number ofstr0groups is present. Grouping itself is correct.keyis unique in the CALCS fixture (17 distinct over 17 documents), sosort keyfixes the selection. Verified per test that 1-shard and 5-shard become identical and every existing expectation still holds:Four other
headtests in this class are deliberately left alone — they assert only schema and non-emptiness, so no exact value depends on the selection.2.
RawFormatIT— rendered table compared row by rowTwo tests compare a fully rendered table against a query with no
ORDER BY:account_numberis unique and ascends in exactly the order already asserted — 1, 6, 13, 18, 20 → Amber, Hattie, Nanette, Dale, Elinor — so ordering by it leaves the expected output byte for byte unchanged. All rows are retained.3.
StandaloneIT— whole response body comparedtestSourceFieldQueryindexes two documents and compares the entire response body,datarowsincluded, against a query with no sort:Sorting by
nameyields the samehello, worldorder the test already expects, so the expected body is unchanged.4.
CalciteConvertCommandIT,CalciteEvalCommandIT,CalciteMVAppendFunctionIT—head Non BANKSeven more tests share the same defect against the seven-row BANK fixture. Sorting by
account_numberselects accounts 1, 6, 13, whose values are exactly what the tests already assert:CalciteConvertCommandIT:testConvertAutoFunction,testConvertNumFunction,testConvertWithAlias,testConvertMultipleFunctions,testConvertNoneFunctionCalciteEvalCommandIT:testEvalStringConcatenationWithExistingDataCalciteMVAppendFunctionIT:testMvappendWithFieldsAndLiterals— the neighbouring real-field tests in that class already sort byaccount_number, so this matches the existing local convention5.
CalciteBinCommandIT—head Nwith exact binstestBinValueFieldOnly:@timestampis unique across all 100time_test_datadocuments, and the three earliest values (8945, 7623, 9187) give exactly the asserted bins8000-10000,6000-8000,8000-10000.testBinSpanWithStartEndNeverShrinkRange: takes six of the seven BANK documents; sorting byaccount_numberyields ages 32, 36, 28, 33, 36, 39 — the existing expectations.testBinTimestampSpan6DaysandtestBinTimestampSpan7Daysare deliberately left failing. They already sort, but they sort afterbinhas replaced@timestampwith the bin label, so every row shares one key and the sort is not a total order. Correcting them needs either a pipeline reorder or new expected values, neither of which belongs in this change.Testing
integTestRemoteagainst the same cluster at both shard counts, to confirm the changes did not trade one failure mode for another:bintimestamp tests excluded aboveCalciteMultiValueStatsITDeliberately not included, since each involves command semantics or engine behaviour rather than an undefined order: the two
bintimestamp tests above,head | sort(testHeadThenSortis already annotatedHEAD_WITHOUT_STABLE_SORTand branches its expectation per route, so moving the sort would change the operation under test),reverse,streamstats,FIRST/LAST,TAKE, and consecutivededup. Nothing requiring an expected-value rewrite is included.Check List
--signoffor-s.Remaining items are not applicable: this changes integration-test queries only, and adds no functionality or user-facing behaviour.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.