Make CalciteReplaceCommandIT column-order-agnostic for analytics-engine route

[RyanL1997]

Description

The analytics-engine route and the v2 / Lucene path return columns in different orders when there is no explicit | fields … clause:

• Parquet preserves the storage order chosen by the on-disk format
• Lucene preserves _source iteration order

Both are valid given the contract verifySchema declares (set equality on column names), so positional verifyDataRows assertions over-constrain the test and fail under -Dtests.analytics.force_routing=true even when the data is semantically correct.

This change applies the same column-name-keyed match pattern @ahkcs introduced for CalcitePPLRenameIT in 59c728b (#5413), now to CalciteReplaceCommandIT.

Changes

1. rowOf(key1, val1, …) — private helper to build column-keyed expected rows preserving insertion order via LinkedHashMap.
2. verifyDataRowsByColumn(…) — looks up each cell value by column name and reorders the expected row to match the response schema before delegating to the existing positional verifyDataRows matcher.
3. Convert four order-sensitive tests to use the new helpers:
   • testMultipleReplace
   • testEmptyStringReplacement
   • testMultipleFieldsInClause
   • testMultiplePairsInSingleCommand
4. Make testReplaceNonExistentField order-agnostic on the input fields are: [...] field list — assert that the prefix and every expected field name appear in the message, but not in a fixed order.

Test results

Against the analytics-engine route with -Dtests.analytics.{force_routing,parquet_indices}=true:

Suite	Before	After
CalciteReplaceCommandIT	16/21 (5 column-order failures)	21/21
CalciteNoPushdownIT > CalciteReplaceCommandIT	16/21	21/21

The v2 path remains green.

Related Issues

• OpenSearch core companion: opensearch-project/OpenSearch#21527 — wires PPL replace command + replace() / regexp_replace() functions on the analytics-engine route via DataFusion's replace / regexp_replace UDFs (Calcite-side wiring + Java-regex→Rust-regex RegexpReplaceAdapter). This SQL PR is the test-side companion: without it the core PR's CalciteReplaceCommandIT validation hits the column-order false-positive failures described above.
• Pattern precedent: Default plugins.calcite.enabled=true on the unified query path #5413 — same rowOf + verifyDataRowsByColumn helper shape applied to CalcitePPLRenameIT.

Check List

• New functionality includes testing.
• All tests pass.
• New functionality has been documented (this PR description; helper javadoc references the rename precedent).
• API changes companion pull request created — N/A.
• Public documentation issue/PR created — N/A.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d853fda)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Error Validation The refactored testReplaceNonExistentField now checks for individual field names like "name", "country", "state" etc. using verifyErrorMessageContains. However, these are very common strings that could appear anywhere in the error message for unrelated reasons. The original test also verified system fields like _id, _index, _score, _maxscore, _sort, _routing — these are no longer checked at all. Consider whether the system fields should also be verified in an order-agnostic way. verifyErrorMessageContains(e, "field [non_existent_field] not found; input fields are:"); verifyErrorMessageContains(e, "name"); verifyErrorMessageContains(e, "country"); verifyErrorMessageContains(e, "state"); verifyErrorMessageContains(e, "month"); verifyErrorMessageContains(e, "year"); verifyErrorMessageContains(e, "age"); Row Count Mismatch The verifyDataRowsByColumn helper only validates that each expected row exists in the response (via verifyDataRows), but does not verify that the total number of rows in the response matches the number of expected rows. If the response returns extra rows, the test would still pass. Confirm that the underlying verifyDataRows already enforces exact row count equality. private final void verifyDataRowsByColumn( JSONObject result, Map<String, Object>... expectedRows) { JSONArray schema = result.getJSONArray("schema"); int n = schema.length(); String[] columnOrder = new String[n]; for (int i = 0; i < n; i++) { columnOrder[i] = schema.getJSONObject(i).getString("name"); } @SuppressWarnings({"unchecked", "rawtypes"}) Matcher<JSONArray>[] rowMatchers = new Matcher[expectedRows.length]; for (int r = 0; r < expectedRows.length; r++) { Object[] reordered = new Object[n]; for (int c = 0; c < n; c++) { if (!expectedRows[r].containsKey(columnOrder[c])) { throw new IllegalArgumentException( "Expected row at index " + r + " is missing canonical value for response column [" + columnOrder[c] + "]; provided keys: " + expectedRows[r].keySet()); } reordered[c] = expectedRows[r].get(columnOrder[c]); } rowMatchers[r] = rows(reordered); } verifyDataRows(result, rowMatchers); Unsafe Cast In rowOf, the key is cast directly with (String) pairs[i] without any null check or type validation. If a non-String key is accidentally passed, this will throw a ClassCastException with a less informative message than a dedicated validation. Consider adding an instanceof check with a descriptive error message. private static Map<String, Object> rowOf(Object... pairs) { if (pairs.length % 2 != 0) { throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); } Map<String, Object> row = new LinkedHashMap<>(); for (int i = 0; i < pairs.length; i += 2) { row.put((String) pairs[i], pairs[i + 1]); } return row;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d853fda

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Tighten field-name substring assertions in error message Checking for bare field names like "name", "age", etc. is too permissive — these substrings could appear anywhere in the error message (e.g., in the field non_existent_field itself or in surrounding text). Use more specific patterns such as "[name," or " name," to ensure the assertions are actually verifying the field list contents. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [260-265] -verifyErrorMessageContains(e, "name"); -verifyErrorMessageContains(e, "country"); -verifyErrorMessageContains(e, "state"); -verifyErrorMessageContains(e, "month"); -verifyErrorMessageContains(e, "year"); +verifyErrorMessageContains(e, "[name,"); +verifyErrorMessageContains(e, "country,"); +verifyErrorMessageContains(e, "state,"); +verifyErrorMessageContains(e, "month,"); +verifyErrorMessageContains(e, "year,"); verifyErrorMessageContains(e, "age"); Suggestion importance[1-10]: 4 __ Why: The concern about bare field names like "name" or "age" matching unintended substrings is valid but low risk in this specific error message context. The improved_code uses patterns like "[name," which are more specific, but the last field "age" still lacks a trailing delimiter, making the fix incomplete.	Low
	Validate expected row key count matches schema column count The method does not validate that the number of columns in the response schema matches the number of keys in each expected row map. If a response returns extra or fewer columns than expected, reordered will be silently under/over-populated. Add a check that n equals the size of each expected row map. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [557-564] private final void verifyDataRowsByColumn( JSONObject result, Map<String, Object>... expectedRows) { -JSONArray schema = result.getJSONArray("schema"); -int n = schema.length(); -String[] columnOrder = new String[n]; -for (int i = 0; i < n; i++) { - columnOrder[i] = schema.getJSONObject(i).getString("name"); -} + JSONArray schema = result.getJSONArray("schema"); + int n = schema.length(); + String[] columnOrder = new String[n]; + for (int i = 0; i < n; i++) { + columnOrder[i] = schema.getJSONObject(i).getString("name"); + } + for (int r = 0; r < expectedRows.length; r++) { + if (expectedRows[r].size() != n) { + throw new IllegalArgumentException( + "Expected row at index " + r + " has " + expectedRows[r].size() + + " keys but schema has " + n + " columns."); + } + } Suggestion importance[1-10]: 4 __ Why: Adding a size check between the expected row map and the schema column count would catch mismatches early with a clear error message. However, the existing code already throws an IllegalArgumentException when a key is missing via containsKey, so the practical impact is limited to providing a slightly earlier and more descriptive failure.	Low

---

Previous suggestions

Suggestions up to commit 4d9046f

Category	Suggestion	Impact
General	Validate expected row key count matches schema columns There is no validation that the number of columns in the response schema matches the number of keys in each expected row map. If a test passes an expectedRows map with more keys than columns in the schema, those extra keys are silently ignored, potentially masking test errors. Add a check that each expected row's key count equals n. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [557-564] private final void verifyDataRowsByColumn( JSONObject result, Map<String, Object>... expectedRows) { -JSONArray schema = result.getJSONArray("schema"); -int n = schema.length(); -String[] columnOrder = new String[n]; -for (int i = 0; i < n; i++) { - columnOrder[i] = schema.getJSONObject(i).getString("name"); -} + JSONArray schema = result.getJSONArray("schema"); + int n = schema.length(); + String[] columnOrder = new String[n]; + for (int i = 0; i < n; i++) { + columnOrder[i] = schema.getJSONObject(i).getString("name"); + } + for (int r = 0; r < expectedRows.length; r++) { + if (expectedRows[r].size() != n) { + throw new IllegalArgumentException( + "Expected row at index " + r + " has " + expectedRows[r].size() + + " keys but schema has " + n + " columns"); + } + } Suggestion importance[1-10]: 5 __ Why: Adding a size check before iterating is a reasonable defensive measure that would catch mismatches between expected row keys and schema columns early. However, the existing code already throws an IllegalArgumentException when a key is missing from expectedRows[r], so extra keys are the only undetected case, making this a minor improvement.	Low
	Tighten field-name assertions to avoid false positives Checking for bare field names like "name", "age", "state" etc. is too broad — these substrings could appear in other parts of the error message (e.g., in the field path non_existent_field contains "field", and future messages could contain these words coincidentally). Consider wrapping each field name with its surrounding bracket notation (e.g., "[name]") to make the assertions more precise and less prone to false positives. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [260-265] -verifyErrorMessageContains(e, "name"); -verifyErrorMessageContains(e, "country"); -verifyErrorMessageContains(e, "state"); -verifyErrorMessageContains(e, "month"); -verifyErrorMessageContains(e, "year"); -verifyErrorMessageContains(e, "age"); +verifyErrorMessageContains(e, "[name]"); +verifyErrorMessageContains(e, "[country]"); +verifyErrorMessageContains(e, "[state]"); +verifyErrorMessageContains(e, "[month]"); +verifyErrorMessageContains(e, "[year]"); +verifyErrorMessageContains(e, "[age]"); Suggestion importance[1-10]: 4 __ Why: The suggestion to use [name] bracket notation is valid for reducing false positives, but the actual error message format uses comma-separated names in a list like [name, country, ...], so individual bracket-wrapped names like [name] may not appear literally in the message. The improvement is marginal and may not work correctly with the actual error format.	Low

Suggestions up to commit 2fca52c

Category	Suggestion	Impact
General	Avoid false positives in substring field name checks Checking for bare field names like "name", "state", "age" etc. is fragile because these substrings could appear in other parts of the error message (e.g., in the field non_existent_field itself, or in index/routing metadata field names like _routing). Consider wrapping each expected field name with surrounding delimiters (e.g., "[name]" or " name,") to ensure only actual field names are matched. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [176-181] -verifyErrorMessageContains(e, "name"); -verifyErrorMessageContains(e, "country"); -verifyErrorMessageContains(e, "state"); -verifyErrorMessageContains(e, "month"); -verifyErrorMessageContains(e, "year"); -verifyErrorMessageContains(e, "age"); +verifyErrorMessageContains(e, "[name]"); +verifyErrorMessageContains(e, "[country]"); +verifyErrorMessageContains(e, "[state]"); +verifyErrorMessageContains(e, "[month]"); +verifyErrorMessageContains(e, "[year]"); +verifyErrorMessageContains(e, "[age]"); Suggestion importance[1-10]: 4 __ Why: The suggestion has merit since bare field names like "name" or "state" could match substrings in other parts of the error message. However, the actual error message format is unknown from the diff, so [name] brackets may not match the actual format either. This is a minor robustness improvement for test code.	Low
	Improve error clarity for misaligned key-value arguments The cast (String) pairs[i] will throw a ClassCastException at runtime if a non-String key is accidentally passed (e.g., an integer value placed in the key position due to argument misalignment). Consider adding an explicit type check or using String.valueOf(pairs[i]) to produce a clearer error message that identifies the problematic argument index. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [421-430] private static Map<String, Object> rowOf(Object... pairs) { -if (pairs.length % 2 != 0) { - throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); -} -Map<String, Object> row = new LinkedHashMap<>(); -for (int i = 0; i < pairs.length; i += 2) { - row.put((String) pairs[i], pairs[i + 1]); -} -return row; + if (pairs.length % 2 != 0) { + throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); + } + Map<String, Object> row = new LinkedHashMap<>(); + for (int i = 0; i < pairs.length; i += 2) { + if (!(pairs[i] instanceof String)) { + throw new IllegalArgumentException( + "rowOf expects String keys, but argument at index " + i + " is: " + pairs[i]); + } + row.put((String) pairs[i], pairs[i + 1]); + } + return row; } Suggestion importance[1-10]: 3 __ Why: This is a defensive improvement for a test helper method rowOf. While the explicit type check would give a clearer error message than a ClassCastException, this is test utility code where misuse would be caught quickly during development, making the improvement low-impact.	Low

Suggestions up to commit e8480cb

Category	Suggestion	Impact
General	Tighten field-name substring assertions in error message Checking for bare field names like "name", "state", "age" etc. in the error message is fragile — these substrings could appear in other parts of the message (e.g., in the error prefix, field values, or index names), producing false positives. Consider wrapping each expected field name with its surrounding bracket syntax (e.g., "[name]") to make the assertions more precise and less prone to accidental matches. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [176-181] -verifyErrorMessageContains(e, "name"); -verifyErrorMessageContains(e, "country"); -verifyErrorMessageContains(e, "state"); -verifyErrorMessageContains(e, "month"); -verifyErrorMessageContains(e, "year"); -verifyErrorMessageContains(e, "age"); +verifyErrorMessageContains(e, "[name]"); +verifyErrorMessageContains(e, "[country]"); +verifyErrorMessageContains(e, "[state]"); +verifyErrorMessageContains(e, "[month]"); +verifyErrorMessageContains(e, "[year]"); +verifyErrorMessageContains(e, "[age]"); Suggestion importance[1-10]: 5 __ Why: The suggestion is valid — bare field names like "name" or "state" could match unintended parts of the error message. However, the actual error format uses a list like [name, country, ...] rather than [name] per field, so wrapping with brackets may not match the actual format either. The improvement is moderate but the proposed format may not align with the real error message structure.	Low
	Detect duplicate keys in row builder The cast (String) pairs[i] will throw a ClassCastException at runtime if a non-String key is accidentally passed. Since the method is called with interleaved string keys and mixed-type values, a duplicate key (same column name passed twice) would silently overwrite the first value without any warning. Consider adding a duplicate-key check to catch test authoring mistakes early. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteReplaceCommandIT.java [421-430] private static Map<String, Object> rowOf(Object... pairs) { -if (pairs.length % 2 != 0) { - throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); -} -Map<String, Object> row = new LinkedHashMap<>(); -for (int i = 0; i < pairs.length; i += 2) { - row.put((String) pairs[i], pairs[i + 1]); -} -return row; + if (pairs.length % 2 != 0) { + throw new IllegalArgumentException("rowOf expects an even number of args (key, value, ...)"); + } + Map<String, Object> row = new LinkedHashMap<>(); + for (int i = 0; i < pairs.length; i += 2) { + String key = (String) pairs[i]; + if (row.containsKey(key)) { + throw new IllegalArgumentException("rowOf: duplicate key [" + key + "]"); + } + row.put(key, pairs[i + 1]); + } + return row; } Suggestion importance[1-10]: 4 __ Why: Adding a duplicate-key check to rowOf is a reasonable defensive measure for a test helper, preventing silent overwrites from test authoring mistakes. However, this is a test utility method and the impact is low since all call sites in the PR use distinct keys.	Low

[github-actions]

Persistent review updated to latest commit 2fca52c

[github-actions]

Persistent review updated to latest commit 4d9046f

[github-actions]

Persistent review updated to latest commit d853fda