Skip to content

test(integ-test): stabilize join subsearch maxout assertion across shards - #5742

Open
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/join-subsearch-maxout-forced-count
Open

test(integ-test): stabilize join subsearch maxout assertion across shards#5742
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/join-subsearch-maxout-forced-count

Conversation

@mengweieric

Copy link
Copy Markdown
Collaborator

Description

CalcitePPLJoinIT.testJoinSubsearchMaxOut passed on a single shard and failed on five with expected:<10> but was:<15>.

The fix: lower the cap so it actually bounds the result, assert that bound on the original query, and add a companion test that filters the subsearch to the join key so the capped count is exact again. Test-only change, no production behavior is touched.

Why the original assertion could not hold

plugins.ppl.join.subsearch_maxout bounds how many subsearch rows are joined against, not which ones. LogicalSystemLimit takes its collation from its input, and an unsorted subsearch has none, so the discarded row is whichever one the scan yields last. The user documentation and join.md both specify only a maximum row count.

The fixture makes that decisive. Three of the six occupation rows match country = 'Canada', and the left side has five matching rows:

cap discards a Canada row      ->  5 x 2 = 10
cap discards a non-Canada row  ->  5 x 3 = 15

Both satisfy a cap of five, so the assertion inferred cap enforcement from the identity of an arbitrarily discarded row. A cap of five cannot bound this result at all, since five already exceeds the three matching rows.

What changed

Test Cap Assertion
testJoinSubsearchMaxOut 2 joined rows <= 10, then exactly 15 uncapped
testJoinSubsearchMaxOutOnFilteredSubsearch (new) 2 exactly 10, then exactly 15 uncapped

With a cap of two, at most two matching rows can survive, so the join cannot exceed 5 x 2 whichever rows are kept. That bound is what the setting actually promises. Filtering the subsearch to the join key makes every retained row join identically, which recovers an exact count without depending on row identity.

Scope notes:

  • One test file. No fixture, mapping, enum or constant is touched, and no new index is added.
  • The original method name and its query are unchanged, so the bare-index right side stays covered under a cap.
  • The uncapped expectation of 15 is unchanged in both tests.
  • No production code changes, and no engine behavior is asserted differently. The cap was already enforced correctly.

Validation

Both tests, both pushdown modes, one and five shards: 8 executions, all pass. Shard counts are confirmed from the cluster log rather than assumed, and each result file was attributed to its pushdown mode by inspecting the recorded setting rather than by fork order.

Cell Before After
5 shards, pushdown on FAIL expected:<10> but was:<15> PASS
5 shards, no pushdown PASS PASS
1 shard, pushdown on PASS PASS
1 shard, no pushdown PASS PASS

The five-shard failure reproduced locally before the change, and only with pushdown enabled. That the same code passes with pushdown disabled is further evidence that the retained subset varies with execution strategy and was never fixed.

The new bound is not vacuous. Mutating only testJoinSubsearchMaxOut's cap to 0 makes it fail in both modes with can join at most 5 x 2 = 10 rows, but got 15, while the companion test keeps passing. So an unenforced cap is still caught.

spotlessJavaCheck, compileTestJava and git diff --check pass.

Related Issues

None. The engine behavior is correct and unchanged; only the test oracle assumed more than the setting guarantees.

Check List

  • New functionality includes testing.
  • Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ards

The join subsearch maxout setting bounds how many subsearch rows are joined
against, not which ones. LogicalSystemLimit takes its collation from its
input, which is empty for an unsorted subsearch, so the discarded row is
whichever one the scan yields last.

testJoinSubsearchMaxOut asserted an exact count that is only reachable when
the cap happens to discard a row matching the join key. Three of the six
right-side rows match, so a cap of five need not discard a matching row at
all, and the result is 10 or 15 depending on scan order. It passed on one
shard and failed on five with expected:<10> but was:<15>.

The cap is now two, below the three matching rows, so at most two can
survive and the joined result cannot exceed 5 x 2 regardless of which rows
are kept. The original query keeps its exact uncapped assertion and gains
that bound. A companion test filters the subsearch to the join key so every
retained row joins identically, which makes the capped count exact again.

No production behavior is changed.

Signed-off-by: Eric Wei <[email protected]>
@mengweieric mengweieric added the testing Related to improving software testing label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add lower bound validation

Add a lower bound check to ensure the capped result is not empty or unexpectedly
small. This prevents false positives if the query returns zero rows due to data
issues or query execution problems.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLJoinIT.java [1137-1140]

 int cappedRows = executeQuery(query).getJSONArray("datarows").length();
 assertTrue(
     "A subsearch capped at 2 rows can join at most 5 x 2 = 10 rows, but got " + cappedRows,
-    cappedRows <= 10);
+    cappedRows > 0 && cappedRows <= 10);
Suggestion importance[1-10]: 5

__

Why: Adding a lower bound check (cappedRows > 0) would make the test more robust by catching cases where the query unexpectedly returns zero rows. However, the test's primary purpose is to verify the upper bound of the cap (≤ 10 rows), and the comment explains that the exact count is non-deterministic due to unsorted subsearch results. The suggestion is valid but represents a moderate improvement in test robustness rather than fixing a critical issue.

Low

Comment on lines +1136 to 1141
setJoinSubsearchMaxOut(2);
int cappedRows = executeQuery(query).getJSONArray("datarows").length();
assertTrue(
"A subsearch capped at 2 rows can join at most 5 x 2 = 10 rows, but got " + cappedRows,
cappedRows <= 10);
resetJoinSubsearchMaxOut();

@dai-chen dai-chen Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do this in try-finally in case setting is not reset when test fails?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to improving software testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants