Move Calcite-only tests from CrossClusterSearchIT to CalciteCrossClusterSearchIT #5085
Conversation
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
…citeCrossClusterSearchIT Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
yuancu
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar and
ykmr1224
as code owners
Contributor
📝 WalkthroughWalkthroughRelocates five cross-cluster search tests that use Calcite-only commands (addtotals, addcoltotals, transpose, append, mvcombine) from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java`:
- Around line 354-361: Update misleading copy-paste comments in the
CalciteCrossClusterSearchIT test methods so they accurately describe the test
action instead of saying "Test query_string without fields parameter";
specifically edit the comment above testCrossClusterAddTotals to mention testing
the addtotals command on a remote cluster, the comment above
testCrossClusterAddColTotals to mention testing addcoltotals, and the comment
above testCrossClusterTranspose to mention testing transpose; locate these
comments in the CalciteCrossClusterSearchIT class near the methods
testCrossClusterAddTotals, testCrossClusterAddColTotals, and
testCrossClusterTranspose and either remove the incorrect lines or replace them
with concise, accurate descriptions referencing the exact command being
verified.
integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java
Show resolved
Hide resolved
…ommands Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java`:
- Around line 353-421: Add null/invalid/boundary and multi-document coverage for
the new cross-cluster Calcite tests by extending the existing test methods (or
adding new tests) for testCrossClusterAddTotals, testCrossClusterAddColTotals,
testCrossClusterTranspose, testCrossClusterAppend, and
testCrossClusterMvcombine: include cases with null field values, empty result
sets, out-of-range/boundary numeric values, and malformed/invalid input strings;
verify results using the same helpers (verifyDataRows, verifyColumn, rows,
columnName) and add multi-document scenarios where the operator is per-document
(e.g., multiple docs with/without target field) and negative tests asserting
proper error messages or empty outputs for invalid inputs. Ensure each new test
asserts NULL handling, boundary numeric sums/concats, and at least one
multi-document aggregation scenario for each operator.
Comment on lines
+353
to
+421
| @Test | ||
| public void testCrossClusterAddTotals() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "search source=%s| sort 1 age | fields firstname, age | addtotals age", | ||
| TEST_INDEX_BANK_REMOTE)); | ||
| verifyDataRows(result, rows("Nanette", 28, 28)); | ||
| } | ||
|
|
||
| /** CrossClusterSearchIT Test for addcoltotals. */ | ||
| @Test | ||
| public void testCrossClusterAddColTotals() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "search source=%s | where firstname='Hattie' or firstname ='Nanette'|fields" | ||
| + " firstname,age,balance | addcoltotals age balance", | ||
| TEST_INDEX_BANK_REMOTE)); | ||
| verifyDataRows( | ||
| result, rows("Hattie", 36, 5686), rows("Nanette", 28, 32838), rows(null, 64, 38524)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCrossClusterTranspose() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "search source=%s | where firstname='Hattie' or firstname ='Nanette' or" | ||
| + " firstname='Dale'|sort firstname desc |fields firstname,age,balance |" | ||
| + " transpose 3 column_name='column_names'", | ||
| TEST_INDEX_BANK_REMOTE)); | ||
|
|
||
| verifyDataRows( | ||
| result, | ||
| rows("firstname", "Nanette", "Hattie", "Dale"), | ||
| rows("balance", "32838", "5686", "4180"), | ||
| rows("age", "28", "36", "33")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCrossClusterAppend() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "search source=%s | stats count() as cnt by gender | append [ search source=%s |" | ||
| + " stats count() as cnt ]", | ||
| TEST_INDEX_BANK_REMOTE, TEST_INDEX_BANK_REMOTE)); | ||
| verifyDataRows(result, rows(3, "F"), rows(4, "M"), rows(7, null)); | ||
| } | ||
|
|
||
| /** CrossClusterSearchIT Test for mvcombine. */ | ||
| @Test | ||
| public void testCrossClusterMvcombine() throws IOException { | ||
|
|
||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "search source=%s | where firstname='Hattie' or firstname='Nanette' " | ||
| + "| fields firstname, age | mvcombine age", | ||
| TEST_INDEX_BANK_REMOTE)); | ||
|
|
||
| verifyColumn(result, columnName("firstname"), columnName("age")); | ||
|
|
||
| verifyDataRows( | ||
| result, | ||
| rows("Hattie", new org.json.JSONArray().put(36)), | ||
| rows("Nanette", new org.json.JSONArray().put(28))); | ||
| } |
Contributor
There was a problem hiding this comment.
Add null/invalid/boundary coverage (or point to existing coverage) for these new Calcite cross-cluster tests.
These additions are happy-path only. Please add null/invalid/boundary cases for addtotals/addcoltotals/transpose/append/mvcombine and ensure multi-document coverage where the command is per-document, or link to existing tests that already cover these scenarios.
As per coding guidelines: “NULL input tests must be included for all new functions”, “Include boundary condition tests (min/max values, empty inputs) for all new functions”, “Include error condition tests (invalid inputs, exceptions) for all new functions”, and “Include multi-document tests for per-document operations”.
🤖 Prompt for AI Agents
In
`@integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java`
around lines 353 - 421, Add null/invalid/boundary and multi-document coverage
for the new cross-cluster Calcite tests by extending the existing test methods
(or adding new tests) for testCrossClusterAddTotals,
testCrossClusterAddColTotals, testCrossClusterTranspose, testCrossClusterAppend,
and testCrossClusterMvcombine: include cases with null field values, empty
result sets, out-of-range/boundary numeric values, and malformed/invalid input
strings; verify results using the same helpers (verifyDataRows, verifyColumn,
rows, columnName) and add multi-document scenarios where the operator is
per-document (e.g., multiple docs with/without target field) and negative tests
asserting proper error messages or empty outputs for invalid inputs. Ensure each
new test asserts NULL handling, boundary numeric sums/concats, and at least one
multi-document aggregation scenario for each operator.
LantaoJin
approved these changes
Jan 29, 2026
qianheng-aws
approved these changes
Jan 29, 2026
Contributor
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5085-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4b785b0cd4ad081129b4ee130d96a0e74fab0b8e
# Push it to GitHub
git push --set-upstream origin backport/backport-5085-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
yuancu
added a commit
that referenced
this pull request
Jan 29, 2026
…terSearchIT (#5085) * Move tests for calcite-only commands from CrossClusterSearchIT to CalciteCrossClusterSearchIT Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Ask to use CalciteCrossClusterSearchIT when developing calcite-only commands Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> --------- Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> (cherry picked from commit 4b785b0)
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
CrossClusterSearchITtoCalciteCrossClusterSearchITCrossClusterSearchIT:testCrossClusterAddTotals- usesaddtotalscommandtestCrossClusterAddColTotals- usesaddcoltotalscommandtestCrossClusterTranspose- usestransposecommandtestCrossClusterAppend- usesappendcommandtestCrossClusterMvcombine- usesmvcombinecommandIssues Resolved
Fixes #5084
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.