[BugFix] Shadow stale mapped leaves when an override replaces an object parent (#5718) by Ystk-hsn · Pull Request #5726 · opensearch-project/sql

Ystk-hsn · 2026-08-27T17:10:08Z

Description

Problem

spath input=body output=log on an index where log is a mapped object field silently answers log.<key> references from the stale mapped leaves instead of the extracted JSON (issue #5718). Within a single row, each leaf independently reads whichever source happens to be mapped — and where log.level = 'ERROR' on the extracted value silently matches nothing.

Root cause

Three interacting behaviors:

OpenSearch exposes an object field to the row schema as a struct-parent column (log) plus flattened leaf columns (log.level, log.src) side by side.
projectPlusOverriding replaces only exact-name matches, so spath's rewrite (eval log = json_extract_all(body)) replaces the parent column but leaves the stale leaf columns in the schema.
QualifiedNameResolver prefers an exact-name column over map access, so log.level resolves to the stale leaf while log.msg (unmapped) falls through to the fresh map.

Fix

Add the mirror of the existing dropStructParentsFor step: when an override replaces a column that was an object/nested parent (MAP/ARRAY-typed), also drop its flattened leaf columns (dropStructChildrenFor). The replacement value then shadows the entire <name>.* subtree — issue #5718's preferred option 1.

The children-drop is type-gated on the replaced column being container-typed. A scalar column that merely shares a dotted prefix with user-created literal columns (eval x.y = 1 | eval x = 2) keeps its children, preserving the SPL1 literal-field semantics that PR #5351 restored. Same gating style as the existing parent-drop: it only fires when the override actually replaced a container column.

This also fixes a companion defect found while reproducing: with stale leaves present, a subsequent eval log.level = 'patched' fired the override path and dropStructParentsFor removed the freshly extracted map (Field [log] not found). With the stale leaves gone, the assignment creates a literal column and the parent survives, consistent with the #5185 semantics.

Behavior changes (only when an assignment collides with a mapped object/nested parent)

Query pattern	Before	After
`spath output=<object parent>` → leaf reference	stale mapped value, silent	extracted value
same → `where` on leaf	silently matches stale value	filters on extracted value
same → unrelated doc triggers dynamic mapping	extraction silently retires to `null`	unaffected
`spath ... path=...` / `eval <parent> = <scalar>` → leaf reference	stale value, silent	clear resolution error (mirrors the flat-keyword collision behavior)
`spath` collision → `eval` a dotted leaf	`Field not found` error	works; parent map survives
user literal dotted columns under a scalar prefix	preserved	preserved (type gate)

Default (fields *) output shape is unchanged — the stale leaves were already hidden by tryToRemoveNestedFields; they were only reachable by explicit reference.

Known accepted corner: a literal dotted column created under a mapped object parent is dropped together with the mapped leaves when the parent itself is overridden (provenance of individual columns is not tracked).

Testing

New CalcitePPLSpathCollisionIT (10 tests) written against the expected semantics before the fix: 7 failed on the unfixed build reproducing every symptom (A/B/C and path-mode) plus the companion defect; 3 guard tests pinned the already-correct behaviors. All 10 pass with the fix.
Reproduced on main (issue was reported against 3.8; confirms main is affected).
Regression suites all green: Spath/Eval/Bin/Rex/Parse/Trendline/Flatten/Patterns/MapPath ITs (278+ tests), :ppl:test spath units, :core:test, :integ-test:yamlRestTest (includes the issues/5185.yml regression).
Documented the collision behavior in docs/user/ppl/cmd/spath.md (prose only; no new doctest blocks).

Related Issues

Resolves #5718

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed. (N/A — not a new command)
API changes companion pull request created. (N/A)
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created. (N/A — in-repo user manual updated)

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.

github-actions · 2026-08-27T17:11:30Z

PR Reviewer Guide 🔍

(Review updated until commit `50a90d5`)

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 · 2026-08-27T17:11:53Z

PR Code Suggestions ✨

Latest suggestions up to 50a90d5

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for field retrieval Add null-safety check before calling `getType()` on the field. If `getField()` returns null (field not found), calling `getType()` will throw a NullPointerException. Wrap the field retrieval in a null check to prevent runtime crashes. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1433-1438] for (String overridden : overriddenNames) { - if (isMappingDerivedContainerType( - originalRowType.getField(overridden, true, false).getType())) { + RelDataTypeField field = originalRowType.getField(overridden, true, false); + if (field != null && isMappingDerivedContainerType(field.getType())) { dropStructChildrenFor(overridden, context); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential `NullPointerException` if `getField()` returns null. However, since `overriddenNames` is filtered from `originalFieldNameSet` which comes from `originalRowType.getFieldNames()`, the field should exist. Still, defensive programming warrants this check.	Medium
Possible issue	Add null check for type parameter Add null-safety check for the `type` parameter at the method entry. If a null `type` is passed, calling `getSqlTypeName()` will throw a NullPointerException. Guard against this by returning false early when type is null. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1479-1489] private static boolean isMappingDerivedContainerType(RelDataType type) { + if (type == null) { + return false; + } if (type.getSqlTypeName() == SqlTypeName.MAP) { RelDataType valueType = type.getValueType(); return valueType != null && valueType.getSqlTypeName() == SqlTypeName.ANY; } if (type.getSqlTypeName() == SqlTypeName.ARRAY) { RelDataType componentType = type.getComponentType(); return componentType != null && componentType.getSqlTypeName() == SqlTypeName.ANY; } return false; } Suggestion importance[1-10]: 6 __ Why: Adding a null check for the `type` parameter prevents potential `NullPointerException` when calling `getSqlTypeName()`. This is a defensive programming practice, though the method is called with `field.getType()` which typically shouldn't return null in this context.	Low

Previous suggestions

Suggestions up to commit c5c745d

Category	Suggestion	Impact
Possible issue	Add null-safety for field lookup Add null-safety check before calling `getType()` on the field result. If `getField()` returns null (field not found), calling `getType()` will throw a NullPointerException. Wrap the type check in a null guard to prevent runtime failures. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1437-1442] for (String overridden : overriddenNames) { - if (isMappingDerivedContainerType( - originalRowType.getField(overridden, true, false).getType())) { + RelDataTypeField field = originalRowType.getField(overridden, true, false); + if (field != null && isMappingDerivedContainerType(field.getType())) { dropStructChildrenFor(overridden, context); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential `NullPointerException` if `getField()` returns null. However, since `overriddenNames` is filtered from `originalFieldNameSet` which comes from `originalRowType.getFieldNames()`, the field should exist. Still, adding defensive null-checking is a reasonable safety improvement.	Medium
Possible issue	Validate parent name before prefix construction Validate that `parentName` is not null or empty before constructing the prefix. An empty or null parent name would create an invalid prefix (e.g., ".") that could match unintended fields, leading to incorrect column pruning. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1507-1518] private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { + if (parentName == null \|\| parentName.isEmpty()) { + return; + } String prefix = parentName + "."; List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); List<RexNode> childrenToDrop = fieldNames.stream() .filter(f -> f.startsWith(prefix)) .map(f -> (RexNode) context.relBuilder.field(f)) .toList(); if (!childrenToDrop.isEmpty()) { context.relBuilder.projectExcept(childrenToDrop); } } Suggestion importance[1-10]: 6 __ Why: Adding validation for null or empty `parentName` is a defensive programming practice that prevents potential bugs. However, since `dropStructChildrenFor` is only called with values from `overriddenNames` (which are filtered from existing field names), the input should always be valid in the current context.	Low

Suggestions up to commit a29ce5b

Category	Suggestion	Impact
Possible issue	Add null-safety for field lookup Add null-safety check before calling `getType()` on the field. If `getField()` returns null for a non-existent field name, calling `getType()` will throw a NullPointerException. Filter out null fields before accessing their type. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1432-1435] Set<String> overriddenContainerParents = overriddenNames.stream() - .filter(name -> isContainerType(originalRowType.getField(name, true, false).getType())) + .map(name -> originalRowType.getField(name, true, false)) + .filter(field -> field != null && isContainerType(field.getType())) + .map(field -> field.getName()) .collect(Collectors.toSet()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential `NullPointerException` if `getField()` returns null. The `overriddenNames` are derived from filtering against `originalFieldNameSet`, which should guarantee existence, but defensive programming is warranted. The improved code properly handles the null case and maintains the field names in the set.	Medium
General	Validate parent name before processing Validate that `parentName` is not null or empty before constructing the prefix. An empty or null parent name would create an invalid prefix that could match unintended fields or cause unexpected behavior in the filtering logic. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1491-1501] private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { + if (parentName == null \|\| parentName.isEmpty()) { + return; + } String prefix = parentName + "."; List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); List<RexNode> childrenToDrop = fieldNames.stream() .filter(f -> f.startsWith(prefix)) .map(f -> (RexNode) context.relBuilder.field(f)) .toList(); if (!childrenToDrop.isEmpty()) { context.relBuilder.projectExcept(childrenToDrop); } } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion. While `parentName` comes from `overriddenNames` which should be non-null/non-empty, adding validation prevents potential issues if the method is called from other contexts in the future. The early return is a clean way to handle invalid input.	Medium

Suggestions up to commit 20a0604

Category	Suggestion	Impact
Possible issue	Add null-safety for field lookup Add null-safety check before calling `getType()` on the field. If `getField()` returns null for a non-existent field name, calling `getType()` will throw a NullPointerException. Filter out null fields before accessing their type. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1432-1435] Set<String> overriddenContainerParents = overriddenNames.stream() - .filter(name -> isContainerType(originalRowType.getField(name, true, false).getType())) + .map(name -> originalRowType.getField(name, true, false)) + .filter(field -> field != null && isContainerType(field.getType())) + .map(field -> field.getName()) .collect(Collectors.toSet()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential `NullPointerException` when `getField()` returns null. The `overriddenNames` are derived from filtering against `originalFieldNameSet`, so fields should exist, but defensive programming is valuable here. The improved code properly handles the null case.	Medium
General	Validate field resolution before casting Verify that `context.relBuilder.field(f)` doesn't return null before casting to `RexNode`. If a field name exists in the row type but cannot be resolved by the builder, this could cause issues. Add validation or filter out unresolvable fields. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1491-1501] private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { String prefix = parentName + "."; List<String> fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); List<RexNode> childrenToDrop = fieldNames.stream() .filter(f -> f.startsWith(prefix)) - .map(f -> (RexNode) context.relBuilder.field(f)) + .map(f -> context.relBuilder.field(f)) + .filter(node -> node != null) + .map(node -> (RexNode) node) .toList(); if (!childrenToDrop.isEmpty()) { context.relBuilder.projectExcept(childrenToDrop); } } Suggestion importance[1-10]: 7 __ Why: This suggestion asks to verify that `context.relBuilder.field(f)` doesn't return null. While the field names come from the row type, adding null-safety is a reasonable defensive measure. However, since this is primarily a verification request rather than fixing a confirmed bug, the score is moderate.	Medium

Ystk-hsn · 2026-09-02T15:41:43Z

The 6 failing unit jobs all hit the licenseHeaders / IBM424_rtl flake on HighlightFunctionIT.java, which was fixed on main by #5733 after this PR's CI ran. A re-run should pick up the fix — could someone trigger one? (All integration, doc, BWC, and security jobs are green.)

dai-chen · 2026-09-03T00:13:50Z

The 6 failing unit jobs all hit the licenseHeaders / IBM424_rtl flake on HighlightFunctionIT.java, which was fixed on main by #5733 after this PR's CI ran. A re-run should pick up the fix — could someone trigger one? (All integration, doc, BWC, and security jobs are green.)

Hi @Ystk-hsn , I think you need to rebase to include the fix from main if you haven't.

…ct parent (opensearch-project#5718) When spath (or any command funnelling through projectPlusOverriding) assigns to a name that collides with a mapped object field, the exact-name override replaced only the struct-parent column and left the flattened leaf columns (log.level, log.src) in the row schema. QualifiedNameResolver prefers an exact-name column, so leaf references silently answered from the stale mapping instead of the extracted value. Mirror the existing dropStructParentsFor step: when the replaced column was container-typed (MAP object parent / ARRAY nested parent), drop its flattened leaf columns so the replacement shadows the entire subtree. The type gate keeps user-created literal dotted columns (eval x.y = 1) independent of scalar prefix overrides, preserving the SPL1 semantics restored in PR opensearch-project#5351. Also fixes the companion defect where a post-spath eval on a stale leaf fired dropStructParentsFor against the freshly extracted map (Field [log] not found). Document the collision behaviour in docs/user/ppl/cmd/spath.md. Signed-off-by: Yasutaka Hisano <yasutennis713@gmail.com>

github-actions · 2026-09-03T04:34:02Z

Persistent review updated to latest commit a29ce5b

Ystk-hsn · 2026-09-03T04:44:22Z

Thanks @dai-chen — rebased onto latest main. All tests pass locally on the rebased branch. A review would also be much appreciated.

dai-chen · 2026-09-03T20:48:51Z

+  }
+
+  /** An OpenSearch object parent surfaces as MAP in the row schema, a nested parent as ARRAY. */
+  private static boolean isContainerType(RelDataType type) {


The gate is a pure type test, so any MAP/ARRAY column counts as a mapped parent, including ones a function just built, which can't have stale flattened leaves. Could you check are both of these returning a row on main and 400 with the changes?

... | eval arr = array(1,2) | eval `arr.x` = 5 | eval arr = array(3,4) | fields arr, `arr.x` main → [[3,4], 5] → Field [arr.x] not found? ... | spath input=body output=data | eval `data.custom` = 'kept' | spath input=body output=data | fields data, `data.custom` main → (map, "kept") → Field [data.custom] not found?

Thanks for the fb.
Confirmed, both queries returned 400 with the previous gate. The gate now checks for the exact type shape the schema conversion produces for mapping-derived parents, which is MAP(VARCHAR, ANY) for object parents and ARRAY(ANY) for nested. Function-built containers carry concrete value/element types, so they no longer trigger the pruning. Both of your scenarios are added to CalcitePPLSpathCollisionIT and now return the same rows as main.
One thing I would like your take on. This is still a type shape heuristic rather than real provenance, so any function whose return type shares that shape is treated as a mapping-derived parent. Scanning the registered functions, map_append is currently the one such case, and the pattern needed to hit it is quite contrived, so I think the heuristic is acceptable for this PR. If a stricter mechanism is needed, it looks like a substantially bigger change, so having a direction from you would be appreciated before I take it on, here or as a follow-up.

Thanks for confirming! If I understand correct, this may be caused by the "parent-child info" missing in our symbol table. If we cannot support such edge case anyway, I think you previous revision by simple type check on parent field is more straightforward.

github-actions · 2026-09-04T13:40:28Z

Persistent review updated to latest commit c5c745d

- Gate the stale-leaf pruning on the exact type shape the OpenSearch schema conversion produces (MAP(VARCHAR, ANY) object parent / ARRAY(ANY) nested parent) instead of any MAP/ARRAY. Function-built containers (array(...), a previous spath result) carry concrete value/element types, cannot have stale flattened leaves, and no longer trigger the pruning — fixes the two reviewer scenarios where a literal dotted column was destroyed. - Run the pruning before any new columns are added (on the original row), so the prefix match can never observe the incoming newNames after rename. - Add both reviewer scenarios to CalcitePPLSpathCollisionIT and a yaml rest test (issues/5718.yml) covering the core collision cases. Signed-off-by: Yasutaka Hisano <yasutennis713@gmail.com>

github-actions · 2026-09-04T14:27:05Z

Persistent review updated to latest commit 50a90d5

dai-chen · 2026-09-04T21:27:29Z

@Ystk-hsn Replied to your comments. Please check the CI failure. Thanks!

Ystk-hsn requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 27, 2026 17:10

Ystk-hsn mentioned this pull request Aug 27, 2026

[BUG] spath output field silently ignored when it collides with an existing object field's mapped subfield #5718

Open

4 tasks

dai-chen added PPL Piped processing language bugFix labels Sep 1, 2026

Ystk-hsn force-pushed the fix/5718-spath-output-collision branch from 20a0604 to a29ce5b Compare September 3, 2026 04:32

dai-chen reviewed Sep 3, 2026

View reviewed changes

Ystk-hsn force-pushed the fix/5718-spath-output-collision branch from c5c745d to 50a90d5 Compare September 4, 2026 14:25

Ystk-hsn requested a review from dai-chen September 4, 2026 14:36

Navigation Menu

Uh oh!

[BugFix] Shadow stale mapped leaves when an override replaces an object parent (#5718) - #5726

[BugFix] Shadow stale mapped leaves when an override replaces an object parent (#5718)#5726
Ystk-hsn wants to merge 2 commits into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision

Ystk-hsn commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 27, 2026 •

edited

Loading

Uh oh!

Ystk-hsn commented Sep 2, 2026 •

edited

Loading

Uh oh!

dai-chen commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

Ystk-hsn commented Sep 3, 2026

Uh oh!

Uh oh!

Uh oh!

dai-chen Sep 3, 2026

Uh oh!

Ystk-hsn Sep 4, 2026

Uh oh!

dai-chen Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

dai-chen commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Ystk-hsn commented Aug 27, 2026

Description

Related Issues

Check List

Uh oh!

github-actions Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 50a90d5)

Uh oh!

github-actions Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Ystk-hsn commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dai-chen commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

Ystk-hsn commented Sep 3, 2026

Uh oh!

Uh oh!

Uh oh!

dai-chen Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Ystk-hsn Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

dai-chen Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

dai-chen commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 27, 2026 •

edited

Loading

(Review updated until commit `50a90d5`)

github-actions Bot commented Aug 27, 2026 •

edited

Loading

Ystk-hsn commented Sep 2, 2026 •

edited

Loading