test(integ-test): fix three multi-shard integration-test defects - #5722
Conversation
The beer fixture is registered with a null mapping, so the index relies on dynamic mapping. LastEditorUserId holds 18 integers and 7 "-1" strings, and whichever value reaches the coordinator's mapping update first decides the inferred type. With more than one shard that order varies, and the bulk load partially fails: mapper [LastEditorUserId] cannot be changed from type [text] to [long] Every suite using the fixture then fails for an unrelated reason -- 22 tests in a 5-shard run, including relevance and pagination suites that merely count beer rows. Pin only that field as long; every string value is numerically coercible, and the remaining fields stay on dynamic mapping so text/keyword behaviour for the relevance suites is unchanged. Signed-off-by: Eric Wei <menwe@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit 3b6bb52)Here are some key observations to aid the review process:
|
6700e67 to
626b2c1
Compare
|
Persistent review updated to latest commit 626b2c1 |
PR Code Suggestions ✨Latest suggestions up to 3b6bb52 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 5467473
Suggestions up to commit 626b2c1
|
setSubsearchMaxOut and setJoinSubsearchMaxOut write transient cluster settings,
and 14 tests across five suites reset them only on the success path. When an
assertion fails the reset never runs and the limit leaks into every later test in
the run. The leaked values are 5, 2, 1, 0 and -1, so a leak either caps later
subsearches or silently makes them unlimited.
These are two independent settings, and each contaminates its own family:
* testJoinSubsearchMaxOut leaks PPL_JOIN_SUBSEARCH_MAXOUT=5, after which later
join tests return exactly 5 rows.
* testSubsearchMaxOut leaks PPL_SUBSEARCH_MAXOUT=1, after which later
IN-subquery tests collapse to exactly 1 row.
Both fail on a 5-shard run, so both leak, and the combined effect looks like a
distributed-execution defect rather than two leaked settings.
Move every reset into a finally block. No assertion or expected value changes.
The query after the finally in testJoinSubsearchMaxOut is intentional -- it
verifies the default was restored and expects 15 rows.
Signed-off-by: Eric Wei <menwe@amazon.com>
Six rex tests assert on datarows[0] of an unsorted result. Row order is unspecified, so on a multi-shard index row 0 is a different document and the assertions fail while the extraction itself is correct. Two are worse than order-dependent. testRexNestedCaptureGroupsBugFix restricts the domain to (pyrami|gmail|yahoo) -- a pattern matching exactly 1 of the 1000 documents -- then takes `head 1`, so unless scan order puts that document first every capture is null and its second half calls getString on a null. That half was also wrapped in a datarows-not-empty conditional, which would let a filtering or rex regression pass vacuously; it now asserts the row is present. Filter to account_number = 1 so each assertion identifies its document, keeping an unfiltered query where cardinality was covered. Expected values are unchanged. Verified that extraction is correct for every row -- concat(user,'@',domain) = email holds 1000/1000 at both 1 and 5 shards -- so narrowing the assertion hides no extraction defect. Signed-off-by: Eric Wei <menwe@amazon.com>
626b2c1 to
5467473
Compare
|
Persistent review updated to latest commit 5467473 |
Track subsearch and join max-out mutations in PPLIntegTestCase and reset dirty settings from an inherited JUnit teardown. Mark settings dirty before update so partially applied changes are still cleaned up, and attempt both resets while preserving cleanup failures.\n\nRestore individual tests to their simple success-path resets; the centralized teardown now provides the failure-path safety net. Signed-off-by: Eric Wei <menwe@amazon.com>
|
Persistent review updated to latest commit 3b6bb52 |
2 participants
Description
Three integration-test defects that only surface when the test index has more than one primary shard. All three are test-side: every changed file is under
src/test, and no production code is touched.These have three different mechanisms, not one:
1. Beer fixture has no mapping
BEERis registered with anullmapping, so the index relies on dynamic mapping.LastEditorUserIdholds 18 integers and 7"-1"strings, and whichever value reaches the coordinator's mapping update first decides the inferred type. That order varies with shard count, so the bulk load partially fails:Real failure, e.g.
CalciteMatchPhrasePrefixIT::required_parameters:Suites then fail for a reason unrelated to what they assert — several merely count beer rows. In a full 5-shard run this accounts for 22 failures; I directly re-verified 13 of them across the relevance and pagination suites I reran, and all 13 now pass.
Pinned as
long; every string value is numerically coercible. No test references the field, so the change is semantically inert, and all other fields stay dynamic sotext/keywordbehaviour is unchanged.2. Subsearch max-out settings leak on failure
setSubsearchMaxOut/setJoinSubsearchMaxOutwrite transient cluster settings, and 14 tests across five suites previously reset them only on the success path — 8 inCalcitePPLExistsSubqueryIT, 3 inCalcitePPLInSubqueryIT, and 1 each inCalcitePPLJoinIT,CalcitePPLScalarSubqueryITandCalciteExplainIT. When an assertion failed, the reset did not run and the limit leaked. The leaked values are5,2,1,0and-1, so a leak either caps later subsearches or silently makes them unlimited.Cleanup is now centralized in
PPLIntegTestCase. The setter methods mark each setting dirty before the cluster update, and an inherited JUnit@Afterresets any dirty setting after the test. Both resets are attempted even if one fails, with cleanup exceptions preserved. Tests keep their explicit success-path resets, while the centralized teardown provides the failure-path safety net without duplicating 14try/finallyblocks.These are two independent settings, and each contaminates its own family:
testJoinSubsearchMaxOutleaksPPL_JOIN_SUBSEARCH_MAXOUT=5, after which later join tests return exactly 5 rows.testSubsearchMaxOutleaksPPL_SUBSEARCH_MAXOUT=1, after which later IN-subquery tests collapse to exactly 1 row.Both fail on a 5-shard run, so both leak, and the combined effect looks like a distributed-execution defect rather than two leaked settings.
Real failures — the two primaries, then a representative cascade of each:
CalcitePPLJoinITCalcitePPLInSubqueryITThose 4 remaining failures are left red on purpose — they are genuine multi-shard failures this PR does not attempt to fix, and were previously buried under the cascade.
testJoinSubsearchMaxOut(expects 10, gets 15) andtestSubsearchMaxOut(which row survivesmax=1) need a separate look.The second query in
testJoinSubsearchMaxOutis intentional: it runs after the explicit mid-test reset, verifies the default was restored, and expects 15 rows.3. Rex tests assert on
datarows[0]Six tests read row 0 of an unsorted result, so on a multi-shard index row 0 is a different document:
Real failures:
The
nullin the last one is the pattern legitimately not matching the document that happened to land first — not an extraction failure.Why this is a test defect and not an engine bug: the row-count assertion passes at both shard counts (1000/1000), so no rows are lost or duplicated; and asserting
concat(user,'@',domain) = emailholds for 1000/1000 rows at both 1 and 5 shards, so extraction is correct on every row. Only the assumption that row 0 is a particular document is wrong.Two are worse than order-dependent:
testRexNestedCaptureGroupsBugFixrestricts the domain to(pyrami|gmail|yahoo)— matching exactly 1 of 1000 documents — then takeshead 1, so unless scan order puts that document first every capture is null and its second half callsgetStringon a null. That half was also wrapped in a datarows-not-empty conditional, which would let a filtering or rex regression pass vacuously; it now asserts the row is present.Fixed by filtering to
account_number = 1so each assertion identifies its document, keeping an unfiltered query where cardinality was covered. Expected values are unchanged — adding a sort instead would have required rewriting three of them, sinceamberdukeisaccount_number1 while the minimum is 0.Testing
integTestRemoteagainst the same cluster at both shard counts, verifying the modified tests did not simply trade one failure mode for another:CalciteRexCommandITNo expected value was changed to match multi-shard output.
Check List
--signoffor-s.Remaining items are not applicable: this changes integration-test fixtures and assertions 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.