Skip to content

test(integ-test): stabilize eval max/min schema assertions across shards - #5740

Open
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/eval-max-min-row-scoped-tests
Open

test(integ-test): stabilize eval max/min schema assertions across shards#5740
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/eval-max-min-row-scoped-tests

Conversation

@mengweieric

Copy link
Copy Markdown
Collaborator

Description

CalcitePPLEvalMaxMinFunctionIT had three tests that passed on a single shard and failed on five.

eval max()/min() carries no plan-time result type, so the reported type of the computed column is taken from the first row of the result set. OpenSearchExecutionEngine does this deliberately as a documented workaround for ANY-typed columns. With one shard the first row is always the same document and the reported type is stable. With more shards the arriving row varies, and because the winning operand differs per row, so does the type.

source=opensearch-sql_test_index_dog | eval new = max(1, 3, age) | fields age, new

age=2  ->  new=3  from the int literal
age=4  ->  new=4  from the bigint field

Both rows are correct. Only the column label moves, between int and bigint, depending on which row arrives first. The failures were always in verifySchema, never in verifyDataRows.

The three affected tests now filter to a single known document, which makes the sampled row deterministic under any shard count. Each one gained a companion test for the other selection direction, so the pair still asserts everything the original two-row query asserted.

Test Query scope Selected value Reported type
testEvalMaxNumeric where age = 2 literal 3 int
testEvalMaxNumericWhenFieldSelected (new) where age = 4 field 4 bigint
testEvalMinNumeric where age = 2 field 2 bigint
testEvalMinNumericWhenLiteralSelected (new) where age = 4 literal 3 int
testEvalMinIgnoresNulls where key = 'values' field 3.1415 double
testEvalMinIgnoresNullsWhenLiteralSelected (new) where key != 'values' literal 5 int

The three tests that assert int are gated on EVAL_MAX_MIN_INT_WIDENING, since the analytics-engine route reports a wider type for the same value.

Scope notes:

  • One test file. No shared fixture, mapping, enum or constant is touched, and no new index is added.
  • The original method names are kept and no test is removed.
  • testEvalMaxIgnoresNulls and the four string and mixed-type tests are unchanged. max(int, 3) is an int on every row, so that test was already shard-stable.
  • The null case still covers both explicit-null and missing-field documents, matching the original row coverage.
  • No production code changes. This makes the assertions shard-independent and does not alter what max() or min() return.

Validation

Run on an external cluster, comparing the same class before and after the change.

Cell Before After
1 shard 8 pass 11 pass
5 shards 5 pass, 3 fail 11 pass, 0 skipped

The five-shard run before the change reproduced exactly the three reported failures, and the cluster log confirms the fixtures were created with five primary shards.

spotlessJavaCheck, compileTestJava and git diff --check pass.

This class is not a member of the CalciteNoPushdownIT suite, so that mode is unaffected. The analytics-engine lane was reasoned from the capability gate rather than executed locally, because that plugin is not available in this tree.

Related Issues

The underlying limitation is that the result type is recovered from data rather than declared at plan time. That is a separate product discussion and is not addressed here.

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.

The eval max()/min() result column has no plan-time type, so the reported
type is taken from the first row of the result set. On a single shard that
row is always the same document, so the assertions were stable. On multiple
shards the arriving row varies and the reported type varies with it, which
failed testEvalMaxNumeric, testEvalMinNumeric and testEvalMinIgnoresNulls.

Each of those tests now filters to a single known document, so the sampled
row is deterministic under any shard count, and a companion test covers the
other selection direction that the original two-row query also asserted.
Values, method names and the remaining tests are unchanged.

The int-asserting tests are gated on EVAL_MAX_MIN_INT_WIDENING because the
analytics-engine route reports a wider type for the same value.

Signed-off-by: Eric Wei <[email protected]>
@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

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