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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

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

Open
Ystk-hsn wants to merge 1 commit into
opensearch-project:mainfrom
Ystk-hsn:fix/5718-spath-output-collision
Open

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

Conversation

@Ystk-hsn

Copy link
Copy Markdown

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:

  1. 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.
  2. 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.
  3. 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 patternBeforeAfter
spath output=<object parent> → leaf referencestale mapped value, silentextracted value
same → where on leafsilently matches stale valuefilters on extracted value
same → unrelated doc triggers dynamic mappingextraction silently retires to nullunaffected
spath ... path=... / eval <parent> = <scalar> → leaf referencestale value, silentclear resolution error (mirrors the flat-keyword collision behavior)
spath collision → eval a dotted leafField not found errorworks; parent map survives
user literal dotted columns under a scalar prefixpreservedpreserved (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.

…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

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

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion 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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugFixPPLPiped processing language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Ystk-hsn@dai-chen