[BugFix] Fix rename with wildcard applying on hidden fields (#5099)

[songkant-aws]

Description

When using rename * as old_* (or any wildcard rename without a preceding fields command), metadata/hidden fields like _id, _index, _score, _maxscore, _sort, and _routing were incorrectly being matched and renamed. This happened because WildcardRenameUtils.matchFieldNames matched all fields in the schema including metadata fields.

Root cause: CalciteRelNodeVisitor.visitRename() passed all field names (including metadata fields) to the wildcard matching utility without filtering.

Fix: After wildcard matching, filter out metadata fields using the existing isMetadataField() check. This ensures wildcard renames only apply to user-visible fields, consistent with how fields and other commands handle metadata fields.

Related Issues

Resolves #5099

Check List

• New functionality includes testing
• Commits signed per DCO (-s)
• spotlessCheck passed
• Unit tests passed
• Integration tests passed

[songkant-aws]

Decision Log

Root Cause: CalciteRelNodeVisitor.visitRename() calls WildcardRenameUtils.matchFieldNames() with the full list of field names from the RelNode schema, which includes OpenSearch metadata fields (_id, _index, _score, _maxscore, _sort, _routing). When the source pattern is a wildcard (e.g., *), all fields including metadata fields are matched and renamed.

Approach: Added a post-filter in visitRename() that removes metadata fields from the wildcard match results using the existing isMetadataField() method. The filter only applies when the source pattern contains wildcards — explicit field renames (e.g., rename _id as something) are unaffected (and already blocked by Rename.validate()). This is consistent with how visitFields and other visitors handle metadata fields.

Alternatives Rejected:

1. Modifying WildcardRenameUtils.matchFieldNames() to accept a filter — rejected because the utility is domain-agnostic and shouldn't know about OpenSearch metadata fields.
2. Pre-filtering newNames before passing to matchFieldNames() — rejected because newNames is also used for non-wildcard exact renames and for index-based operations later; filtering it would change semantics.

Pitfalls: None encountered. The fix is minimal and only touches the wildcard rename path.

Things to Watch: If new metadata fields are added to OpenSearchConstants.METADATAFIELD_TYPE_MAP, they will automatically be excluded from wildcard renames. No follow-up needed.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b8297ce)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix wildcard rename applying on metadata fields Relevant files: core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java integ-test/src/test/java/org/opensearch/sql/security/CalciteCrossClusterSearchIT.java integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java Sub-PR theme: Add YAML REST test for issue 5099 Relevant files: integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5099.yml

⚡ Recommended focus areas for review Incomplete verifyColumn The verifyColumn call now only checks for old_dog_name, old_holdersName, and old_age, but the closing parenthesis is placed before verifySchema. This may indicate a formatting/syntax issue where verifyColumn is missing its closing parenthesis on the correct line, potentially causing a compilation error or incorrect test behavior. verifyColumn( result, columnName("old_dog_name"), columnName("old_holdersName"), columnName("old_age")); verifySchema( result, schema("old_dog_name", "string"), schema("old_holdersName", "string"), schema("old_age", "bigint")); Non-wildcard edge case The metadata field filtering is only applied when isWildcardPattern(sourcePattern) is true. However, matchFieldNames may also return metadata fields for non-wildcard patterns if the source pattern literally matches a metadata field name (e.g., rename _id as myId). Consider whether this is intentional or if the guard condition is too narrow. if (WildcardRenameUtils.isWildcardPattern(sourcePattern)) { matchingFields.removeIf(this::isMetadataField); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b8297ce
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix row values order to match schema The column order in verifySchema is old_name, old_age, old_state, old_country, old_year, old_month, but the rows(...) calls pass values in a different order (name, country, state, month, year, age). This mismatch will cause test failures or incorrect validation. Ensure the row values match the schema column order exactly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [214-219] verifyDataRows( result, - rows("Jake", "USA", "California", 4, 2023, 70), - rows("Hello", "USA", "New York", 4, 2023, 30), - rows("John", "Canada", "Ontario", 4, 2023, 25), - rows("Jane", "Canada", "Quebec", 4, 2023, 20)); + rows("Jake", 70, "California", "USA", 2023, 4), + rows("Hello", 30, "New York", "USA", 2023, 4), + rows("John", 25, "Ontario", "Canada", 2023, 4), + rows("Jane", 20, "Quebec", "Canada", 2023, 4)); Suggestion importance[1-10]: 7 __ Why: The schema order is old_name, old_age, old_state, old_country, old_year, old_month, but the rows(...) calls appear to pass values in a different order (e.g., "Jake", "USA", "California", 4, 2023, 70 suggests name, country, state, month, year, age). This mismatch could cause test failures or incorrect validation.	Medium
General	Strengthen metadata exclusion test assertions The test renames fields matching * (metadata fields) to meta, but then verifies that the schema still contains the original non-metadata field names (name, age, etc.) without any meta_ prefixed columns. If metadata fields like _id, index etc. exist and are excluded from renaming, the schema should remain unchanged — but the test should also verify that no meta columns appear, or at least assert the absence of renamed metadata fields to confirm the fix works correctly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [223-235] public void testRenamePartialWildcardExcludesMetadataFields() throws IOException { JSONObject result = executeQuery(String.format("source = %s | rename _* as meta_*", TEST_INDEX_STATE_COUNTRY)); + // Metadata fields should be excluded from wildcard rename, so schema should be unchanged verifySchema( result, schema("name", "string"), schema("age", "int"), schema("state", "string"), schema("country", "string"), schema("year", "int"), schema("month", "int")); + // Verify no meta_ prefixed columns exist (metadata fields were not renamed) + JSONArray schema = result.getJSONArray("schema"); + for (int i = 0; i < schema.length(); i++) { + assertFalse(schema.getJSONObject(i).getString("name").startsWith("meta_")); + } verifyStandardDataRows(result); } Suggestion importance[1-10]: 2 __ Why: The suggestion adds an extra assertion to verify no meta_* columns exist, but verifySchema already validates the exact schema columns present, making the additional check redundant. The improved code also adds a comment which is excluded from scoring consideration.	Low

---

Previous suggestions

Suggestions up to commit 236b068

Category	Suggestion	Impact
Possible issue	Fix mismatched column order in test rows The column order in verifySchema lists old_name, old_age, old_state, old_country, old_year, old_month, but the rows(...) calls pass values in a different order (name, country, state, month, year, age). This mismatch will cause the test to either fail or silently validate incorrect data. Ensure the argument order in each rows(...) call matches the schema column order. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [214-219] verifyDataRows( result, - rows("Jake", "USA", "California", 4, 2023, 70), - rows("Hello", "USA", "New York", 4, 2023, 30), - rows("John", "Canada", "Ontario", 4, 2023, 25), - rows("Jane", "Canada", "Quebec", 4, 2023, 20)); + rows("Jake", 70, "California", "USA", 2023, 4), + rows("Hello", 30, "New York", "USA", 2023, 4), + rows("John", 25, "Ontario", "Canada", 2023, 4), + rows("Jane", 20, "Quebec", "Canada", 2023, 4)); Suggestion importance[1-10]: 7 __ Why: The schema order is old_name, old_age, old_state, old_country, old_year, old_month, but the rows(...) calls pass values as (name, country, state, month, year, age), which is a mismatch that could cause test failures or silent incorrect validation. This is a real potential bug in the test.	Medium
General	Strengthen test assertion for excluded metadata fields The test testRenamePartialWildcardExcludesMetadataFields renames fields matching (metadata fields like _id, index) to meta, but then verifies that the schema still contains the original non-metadata field names without any meta prefixed columns. If the rename of metadata fields is excluded (as per the fix), the schema should remain unchanged, which is what is being verified — but the test name and intent suggest it should also confirm that no meta_ columns appear. Consider adding an assertion that no meta_ fields exist in the result schema to make the test more robust. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLRenameIT.java [223-235] public void testRenamePartialWildcardExcludesMetadataFields() throws IOException { JSONObject result = executeQuery(String.format("source = %s | rename _* as meta_*", TEST_INDEX_STATE_COUNTRY)); verifySchema( result, schema("name", "string"), schema("age", "int"), schema("state", "string"), schema("country", "string"), schema("year", "int"), schema("month", "int")); + // Confirm no meta_* fields were introduced from metadata fields + JSONArray schema = result.getJSONArray("schema"); + for (int i = 0; i < schema.length(); i++) { + assertFalse(schema.getJSONObject(i).getString("name").startsWith("meta_")); + } verifyStandardDataRows(result); } Suggestion importance[1-10]: 3 __ Why: The verifySchema call already implicitly verifies no meta_* fields exist by checking the exact schema. Adding an explicit loop assertion is redundant and adds complexity without meaningful additional coverage.	Low

[github-actions]

Persistent review updated to latest commit b8297ce