From a29ce5b0e2429849e7adb19ebf1d3b40cb0e4f56 Mon Sep 17 00:00:00 2001 From: Yasutaka Hisano Date: Fri, 28 Aug 2026 02:00:18 +0900 Subject: [PATCH 1/3] [BugFix] Shadow stale mapped leaves when an override replaces an object parent (#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 #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 --- .../sql/calcite/CalciteRelNodeVisitor.java | 45 +++- docs/user/ppl/cmd/spath.md | 2 + .../remote/CalcitePPLSpathCollisionIT.java | 203 ++++++++++++++++++ 3 files changed, 248 insertions(+), 2 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index 2c5a6aadcf7..72634094df4 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -1423,10 +1423,16 @@ private RelNode buildConversionProjection(ConversionState state, CalcitePlanCont void projectPlusOverriding( List newFields, List newNames, CalcitePlanContext context) { - Set originalFieldNameSet = - new HashSet<>(context.relBuilder.peek().getRowType().getFieldNames()); + RelDataType originalRowType = context.relBuilder.peek().getRowType(); + Set originalFieldNameSet = new HashSet<>(originalRowType.getFieldNames()); List overriddenNames = newNames.stream().filter(originalFieldNameSet::contains).toList(); + // Which overridden columns were container-typed (object/nested parent) — captured before + // the replacement, this gates the stale-leaf pruning in step 6 (issue #5718). + Set overriddenContainerParents = + overriddenNames.stream() + .filter(name -> isContainerType(originalRowType.getField(name, true, false).getType())) + .collect(Collectors.toSet()); List toOverrideList = overriddenNames.stream().map(a -> (RexNode) context.relBuilder.field(a)).toList(); // 1. add the new fields, For example "age0, country0" @@ -1455,8 +1461,43 @@ void projectPlusOverriding( // column, so overriddenNames is empty and the struct-parent `agent` survives untouched. // It also keeps issue #5185 safe — spath introduces a MAP root and subsequent eval assigns // to brand-new dotted paths that were not already in the row schema. + // + // The mirror direction (issue #5718): when the replaced column was itself an object/nested + // parent, its stale flattened leaves are pruned in dropStructChildrenFor so the new value + // shadows the whole subtree. for (String overridden : overriddenNames) { dropStructParentsFor(overridden, context); + if (overriddenContainerParents.contains(overridden)) { + dropStructChildrenFor(overridden, context); + } + } + } + + /** An OpenSearch object parent surfaces as MAP in the row schema, a nested parent as ARRAY. */ + private static boolean isContainerType(RelDataType type) { + return type.isStruct() + || type.getSqlTypeName() == SqlTypeName.MAP + || type.getSqlTypeName() == SqlTypeName.ARRAY; + } + + /** + * Mirror of {@link #dropStructParentsFor(String, CalcitePlanContext)} for issue #5718: when an + * override replaced an object/nested parent column (e.g. {@code spath input=body output=log} with + * mapped {@code log.*} subfields), drop the stale flattened leaf columns so the replacement + * shadows the entire subtree. Only invoked when the replaced column was container-typed, which + * keeps user-created literal dotted columns under a scalar prefix untouched. No-op when no such + * child columns exist. + */ + private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { + String prefix = parentName + "."; + List fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); + List childrenToDrop = + fieldNames.stream() + .filter(f -> f.startsWith(prefix)) + .map(f -> (RexNode) context.relBuilder.field(f)) + .toList(); + if (!childrenToDrop.isEmpty()) { + context.relBuilder.projectExcept(childrenToDrop); } } diff --git a/docs/user/ppl/cmd/spath.md b/docs/user/ppl/cmd/spath.md index 94e7a385963..f51daa1afa8 100644 --- a/docs/user/ppl/cmd/spath.md +++ b/docs/user/ppl/cmd/spath.md @@ -26,6 +26,8 @@ The `spath` command supports the following parameters. | `output` | Optional | The destination field in which the extracted data is stored. Default is the value of `path` in path-based mode, or the value of `input` in auto-extract mode. | | `path` | Optional | The JSON path that identifies the data to extract. When omitted, all fields are extracted into a map (auto-extract mode). | +> **Note**: When `output` names an existing field, the extracted result replaces that field entirely, including any mapped subfields: after `spath input=body output=log`, every `log.` reference reads from the extracted value, and keys that exist only in the index mapping resolve to `null` (or raise an error if the extracted value is not an object). To keep both the extracted and the original values readable, use a non-colliding `output` name. + For more information about path syntax, see [json_extract](../functions/json.md#json_extract). ## Auto-extract mode (experimental) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java new file mode 100644 index 00000000000..d45873b1be8 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java @@ -0,0 +1,203 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import static org.junit.Assert.assertThrows; +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; + +import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.client.Request; +import org.opensearch.client.ResponseException; +import org.opensearch.sql.legacy.TestUtils; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +/** + * Behavioural contract for issue #5718 — {@code spath} (and any command that funnels through {@code + * projectPlusOverriding}) assigning to a name that collides with an existing mapped object + * field. + * + *

Expected semantics (issue #5718, preferred option): overriding an object parent shadows the + * entire {@code .*} subtree. After {@code spath input=body output=log}, every {@code + * log.} reference reads from the freshly extracted value; stale mapped leaves must never be + * silently readable. This matches the flat-keyword collision case, which either returns the + * extracted value or raises a clear error — never a silent per-leaf mix. + */ +public class CalcitePPLSpathCollisionIT extends PPLIntegTestCase { + + private static final String COLLISION_INDEX = "test_spath_collision"; + private static final String DYNAMIC_INDEX = "test_spath_collision_dyn"; + + /** + * Explicit mapping mirroring issue #5718: {@code log} is an object with mapped keyword leaves + * {@code log.level} / {@code log.src}, while {@code body} holds a JSON string whose {@code level} + * key collides with the mapped leaf. + */ + private static final String COLLISION_MAPPING = + "{\"mappings\": {\"properties\": {" + + "\"log\": {\"properties\": {" + + "\"level\": {\"type\": \"keyword\"}, \"src\": {\"type\": \"keyword\"}}}," + + "\"body\": {\"type\": \"text\"}}}}"; + + private static final String COLLISION_DOC = + "{\"log\": {\"level\": \"MAPPED-DEBUG\", \"src\": \"real-object\"}," + + " \"body\": \"{\\\"level\\\":\\\"ERROR\\\",\\\"msg\\\":\\\"from json\\\"}\"}"; + + @Override + public void init() throws Exception { + super.init(); + enableCalcite(); + + if (!TestUtils.isIndexExist(client(), COLLISION_INDEX)) { + TestUtils.createIndexByRestClient(client(), COLLISION_INDEX, COLLISION_MAPPING); + Request doc = new Request("PUT", "/" + COLLISION_INDEX + "/_doc/1?refresh=true"); + doc.setJsonEntity(COLLISION_DOC); + client().performRequest(doc); + } + + // Separate index for the dynamic-mapping stability test: doc 2 dynamically maps `log.msg`, + // which must not change what doc 1's `log.msg` reads after extraction. + if (!TestUtils.isIndexExist(client(), DYNAMIC_INDEX)) { + TestUtils.createIndexByRestClient(client(), DYNAMIC_INDEX, COLLISION_MAPPING); + Request doc1 = new Request("PUT", "/" + DYNAMIC_INDEX + "/_doc/1?refresh=true"); + doc1.setJsonEntity(COLLISION_DOC); + client().performRequest(doc1); + Request doc2 = new Request("PUT", "/" + DYNAMIC_INDEX + "/_doc/2?refresh=true"); + doc2.setJsonEntity( + "{\"log\": {\"level\": \"X\", \"src\": \"y\", \"msg\": \"DYNAMICALLY-MAPPED\"}," + + " \"body\": \"{\\\"level\\\":\\\"E2\\\",\\\"msg\\\":\\\"json-2\\\"}\"}"); + client().performRequest(doc2); + } + } + + @Test + public void testCollidingOutputLeafReadsExtractedValue() throws IOException { + // Issue #5718 core case: log.level must read the extracted ERROR, not the stale mapped + // MAPPED-DEBUG. The whole log.* subtree reads from the extraction: log.msg exists only in + // the JSON (-> "from json"), log.src exists only in the stale mapping (-> null). + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log | fields log.level, log.msg, log.src", + COLLISION_INDEX)); + verifyDataRows(result, rows("ERROR", "from json", null)); + } + + @Test + public void testCollidingOutputParentReadsExtractedMap() throws IOException { + // Guard (already true before the fix): the parent reference returns the extracted map. + JSONObject result = + executeQuery( + String.format("source=%s | spath input=body output=log | fields log", COLLISION_INDEX)); + verifyDataRows(result, rows(ImmutableMap.of("level", "ERROR", "msg", "from json"))); + } + + @Test + public void testCollidingOutputWhereMatchesExtractedValue() throws IOException { + // Issue #5718 symptom B: filtering on the extracted value must match. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log | where log.level = 'ERROR' | fields" + + " log.level", + COLLISION_INDEX)); + verifyDataRows(result, rows("ERROR")); + } + + @Test + public void testCollidingOutputWhereStaleValueMatchesNothing() throws IOException { + // The stale mapped value is shadowed and must no longer be reachable through log.level. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log | where log.level = 'MAPPED-DEBUG' |" + + " fields log.level", + COLLISION_INDEX)); + verifyDataRows(result); + } + + @Test + public void testCollidingOutputStableUnderDynamicMapping() throws IOException { + // Issue #5718 symptom C: indexing an unrelated document that dynamically maps `log.msg` + // must not change what the original document's `log.msg` reads. Both rows read from their + // own extracted JSON. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log | fields log.level, log.msg", + DYNAMIC_INDEX)); + verifyDataRows(result, rows("ERROR", "from json"), rows("E2", "json-2")); + } + + @Test + public void testCollidingOutputThenEvalDottedLeaf() throws IOException { + // Companion defect uncovered while reproducing #5718: with stale leaves present, a + // subsequent `eval log.level = ...` fired the override path and dropStructParentsFor + // removed the freshly extracted map (`Field [log] not found`). Expected: the assignment + // creates the literal column and the extracted parent survives — same semantics as the + // non-colliding case guarded by issue #5185. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log | eval `log.level` = 'patched' | fields" + + " log, `log.level`", + COLLISION_INDEX)); + verifyDataRows(result, rows(ImmutableMap.of("level", "ERROR", "msg", "from json"), "patched")); + } + + @Test + public void testCollidingOutputPathModeParentReadsExtractedValue() throws IOException { + // Path mode with a colliding output overrides `log` with the scalar extraction result. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=log path=level | fields log", + COLLISION_INDEX)); + verifyDataRows(result, rows("ERROR")); + } + + @Test + public void testCollidingOutputPathModeLeafIsNotSilentlyReadable() { + // Path mode: `log` is now a scalar, so `log.level` has nothing to resolve against. It must + // not silently answer from the stale mapped leaf; a clear error mirrors the flat-keyword + // collision behaviour described in issue #5718. + assertThrows( + ResponseException.class, + () -> + executeQuery( + String.format( + "source=%s | spath input=body output=log path=level | fields log.level", + COLLISION_INDEX))); + } + + @Test + public void testScalarEvalOverObjectParentIsNotSilentlyReadable() { + // Generalisation of #5718 beyond spath: overriding a mapped object parent with a scalar + // must not leave stale leaves silently readable. `log` is an INTEGER after the eval, so a + // `log.level` reference raises a clear error instead of returning MAPPED-DEBUG. + assertThrows( + ResponseException.class, + () -> + executeQuery( + String.format("source=%s | eval log = 1 | fields log.level", COLLISION_INDEX))); + } + + @Test + public void testLiteralDottedColumnSurvivesScalarParentOverride() throws IOException { + // SPL1 guard (reviewer's case on PR #5351 family): a user-created literal dotted column is + // an independent field. Overriding its scalar name prefix must NOT remove it — subtree + // shadowing only applies when the overridden column was an object/map parent. + JSONObject result = + executeQuery( + String.format( + "source=%s | eval `body.x` = 7 | eval body = 'replaced' | fields body, `body.x`", + COLLISION_INDEX)); + verifyDataRows(result, rows("replaced", 7)); + } +} From 50a90d5fd33724f1063d580ab6ec99de0f136ebc Mon Sep 17 00:00:00 2001 From: Yasutaka Hisano Date: Fri, 4 Sep 2026 18:23:57 +0900 Subject: [PATCH 2/3] Address review: narrow the pruning gate to mapping-derived containers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../sql/calcite/CalciteRelNodeVisitor.java | 55 +++++++----- .../remote/CalcitePPLSpathCollisionIT.java | 30 +++++++ .../rest-api-spec/test/issues/5718.yml | 89 +++++++++++++++++++ 3 files changed, 151 insertions(+), 23 deletions(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5718.yml diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index 72634094df4..e62053a28c7 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -1427,12 +1427,15 @@ void projectPlusOverriding( Set originalFieldNameSet = new HashSet<>(originalRowType.getFieldNames()); List overriddenNames = newNames.stream().filter(originalFieldNameSet::contains).toList(); - // Which overridden columns were container-typed (object/nested parent) — captured before - // the replacement, this gates the stale-leaf pruning in step 6 (issue #5718). - Set overriddenContainerParents = - overriddenNames.stream() - .filter(name -> isContainerType(originalRowType.getField(name, true, false).getType())) - .collect(Collectors.toSet()); + // Issue #5718: an override replacing a mapping-derived parent sheds the stale flattened + // leaves the scan exposed alongside it. Runs before any new columns are added, so the + // prefix match only ever sees pre-existing columns — never the incoming newNames. + for (String overridden : overriddenNames) { + if (isMappingDerivedContainerType( + originalRowType.getField(overridden, true, false).getType())) { + dropStructChildrenFor(overridden, context); + } + } List toOverrideList = overriddenNames.stream().map(a -> (RexNode) context.relBuilder.field(a)).toList(); // 1. add the new fields, For example "age0, country0" @@ -1461,32 +1464,38 @@ void projectPlusOverriding( // column, so overriddenNames is empty and the struct-parent `agent` survives untouched. // It also keeps issue #5185 safe — spath introduces a MAP root and subsequent eval assigns // to brand-new dotted paths that were not already in the row schema. - // - // The mirror direction (issue #5718): when the replaced column was itself an object/nested - // parent, its stale flattened leaves are pruned in dropStructChildrenFor so the new value - // shadows the whole subtree. for (String overridden : overriddenNames) { dropStructParentsFor(overridden, context); - if (overriddenContainerParents.contains(overridden)) { - dropStructChildrenFor(overridden, context); - } } } - /** An OpenSearch object parent surfaces as MAP in the row schema, a nested parent as ARRAY. */ - private static boolean isContainerType(RelDataType type) { - return type.isStruct() - || type.getSqlTypeName() == SqlTypeName.MAP - || type.getSqlTypeName() == SqlTypeName.ARRAY; + /** + * Whether this is the type shape {@code OpenSearchTypeFactory.convertExprTypeToRelDataType} + * produces for a mapping-derived parent: an object parent surfaces as {@code MAP(VARCHAR, ANY)}, + * a nested parent as {@code ARRAY(ANY)}. Function-built containers carry concrete value/element + * types (e.g. {@code json_extract_all} returns {@code MAP(VARCHAR, VARCHAR)}, {@code array(1,2)} + * an {@code ARRAY(INTEGER)}) and are deliberately excluded. + */ + private static boolean isMappingDerivedContainerType(RelDataType type) { + 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; } /** * Mirror of {@link #dropStructParentsFor(String, CalcitePlanContext)} for issue #5718: when an - * override replaced an object/nested parent column (e.g. {@code spath input=body output=log} with - * mapped {@code log.*} subfields), drop the stale flattened leaf columns so the replacement - * shadows the entire subtree. Only invoked when the replaced column was container-typed, which - * keeps user-created literal dotted columns under a scalar prefix untouched. No-op when no such - * child columns exist. + * override replaces a mapping-derived parent column (e.g. {@code spath input=body output=log} + * with mapped {@code log.*} subfields), drop the stale flattened leaf columns so the replacement + * shadows the entire subtree. Only invoked when the replaced column passes {@link + * #isMappingDerivedContainerType(RelDataType)}, which keeps user-created literal dotted columns + * under function-built containers or scalar prefixes untouched. No-op when no such child columns + * exist. */ private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { String prefix = parentName + "."; diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java index d45873b1be8..8b90f3f2a42 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java @@ -200,4 +200,34 @@ public void testLiteralDottedColumnSurvivesScalarParentOverride() throws IOExcep COLLISION_INDEX)); verifyDataRows(result, rows("replaced", 7)); } + + @Test + public void testFunctionBuiltContainerOverridePreservesLiteralDottedColumn() + throws IOException { + // Reviewer's case (a) on PR #5726: `arr` here is a function-built ARRAY, not a mapped + // parent — it cannot have stale flattened leaves, so overriding it must not touch the + // user-created literal column `arr.x`. + JSONObject result = + executeQuery( + String.format( + "source=%s | eval arr = array(1,2) | eval `arr.x` = 5 | eval arr = array(3,4) |" + + " fields arr, `arr.x`", + COLLISION_INDEX)); + verifyDataRows(result, rows(new org.json.JSONArray(java.util.List.of(3, 4)), 5)); + } + + @Test + public void testRepeatedSpathPreservesInterveningLiteralDottedColumn() throws IOException { + // Reviewer's case (b) on PR #5726: the second spath overrides `data`, which at that point + // is the extraction MAP from the first spath (function-built, no stale mapped leaves). + // The literal column `data.custom` created in between must survive. + JSONObject result = + executeQuery( + String.format( + "source=%s | spath input=body output=data | eval `data.custom` = 'kept' | spath" + + " input=body output=data | fields data, `data.custom`", + COLLISION_INDEX)); + verifyDataRows( + result, rows(ImmutableMap.of("level", "ERROR", "msg", "from json"), "kept")); + } } diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5718.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5718.yml new file mode 100644 index 00000000000..07262a14409 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5718.yml @@ -0,0 +1,89 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: true + - do: + indices.create: + index: issue5718 + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + log: + properties: + level: + type: keyword + src: + type: keyword + body: + type: text + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "issue5718", "_id": "1"}}' + - '{"log": {"level": "MAPPED-DEBUG", "src": "real-object"}, "body": "{\"level\":\"ERROR\",\"msg\":\"from json\"}"}' + +--- +teardown: + - do: + indices.delete: + index: issue5718 + ignore_unavailable: true + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: false + +--- +"Issue 5718: spath output colliding with a mapped object parent shadows the whole subtree": + - skip: + features: + - headers + - allowed_warnings + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=issue5718 | spath input=body output=log | fields log.level, log.msg, log.src" + + - match: { total: 1 } + - match: { datarows: [["ERROR", "from json", null]] } + +--- +"Issue 5718: where on the extracted value matches instead of the stale mapped value": + - skip: + features: + - headers + - allowed_warnings + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=issue5718 | spath input=body output=log | where log.level = 'ERROR' | fields log.level" + + - match: { total: 1 } + - match: { datarows: [["ERROR"]] } + +--- +"Issue 5718: the stale mapped value is no longer silently reachable through the leaf": + - skip: + features: + - headers + - allowed_warnings + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=issue5718 | spath input=body output=log | where log.level = 'MAPPED-DEBUG' | fields log.level" + + - match: { total: 0 } + - length: { datarows: 0 } From 2d38143d0b3d19689e91424ceb138301a3275d80 Mon Sep 17 00:00:00 2001 From: Yasutaka Hisano Date: Sat, 5 Sep 2026 16:38:16 +0900 Subject: [PATCH 3/3] Address review: revert to the simple container type check Per review discussion, the row schema carries no parent-child provenance, so distinguishing mapping-derived parents by type shape cannot be made exact anyway. Overriding any container-typed column now consistently shadows its dotted subtree, and the two reviewer scenarios pin that behavior in CalcitePPLSpathCollisionIT. Also fixes the spotless violation in the IT that broke the unit CI jobs. Signed-off-by: Yasutaka Hisano --- .../sql/calcite/CalciteRelNodeVisitor.java | 37 +++++---------- .../remote/CalcitePPLSpathCollisionIT.java | 47 +++++++++---------- 2 files changed, 34 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java index e62053a28c7..31afde1a946 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -1427,12 +1427,11 @@ void projectPlusOverriding( Set originalFieldNameSet = new HashSet<>(originalRowType.getFieldNames()); List overriddenNames = newNames.stream().filter(originalFieldNameSet::contains).toList(); - // Issue #5718: an override replacing a mapping-derived parent sheds the stale flattened + // Issue #5718: an override replacing a container-typed parent sheds the stale flattened // leaves the scan exposed alongside it. Runs before any new columns are added, so the // prefix match only ever sees pre-existing columns — never the incoming newNames. for (String overridden : overriddenNames) { - if (isMappingDerivedContainerType( - originalRowType.getField(overridden, true, false).getType())) { + if (isContainerType(originalRowType.getField(overridden, true, false).getType())) { dropStructChildrenFor(overridden, context); } } @@ -1469,33 +1468,19 @@ void projectPlusOverriding( } } - /** - * Whether this is the type shape {@code OpenSearchTypeFactory.convertExprTypeToRelDataType} - * produces for a mapping-derived parent: an object parent surfaces as {@code MAP(VARCHAR, ANY)}, - * a nested parent as {@code ARRAY(ANY)}. Function-built containers carry concrete value/element - * types (e.g. {@code json_extract_all} returns {@code MAP(VARCHAR, VARCHAR)}, {@code array(1,2)} - * an {@code ARRAY(INTEGER)}) and are deliberately excluded. - */ - private static boolean isMappingDerivedContainerType(RelDataType type) { - 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; + /** An OpenSearch object parent surfaces as MAP in the row schema, a nested parent as ARRAY. */ + private static boolean isContainerType(RelDataType type) { + return type.isStruct() + || type.getSqlTypeName() == SqlTypeName.MAP + || type.getSqlTypeName() == SqlTypeName.ARRAY; } /** * Mirror of {@link #dropStructParentsFor(String, CalcitePlanContext)} for issue #5718: when an - * override replaces a mapping-derived parent column (e.g. {@code spath input=body output=log} - * with mapped {@code log.*} subfields), drop the stale flattened leaf columns so the replacement - * shadows the entire subtree. Only invoked when the replaced column passes {@link - * #isMappingDerivedContainerType(RelDataType)}, which keeps user-created literal dotted columns - * under function-built containers or scalar prefixes untouched. No-op when no such child columns - * exist. + * override replaces a container-typed column (e.g. {@code spath input=body output=log} with + * mapped {@code log.*} subfields), drop the flattened leaf columns so the replacement shadows the + * entire dotted subtree. The row schema carries no parent-child provenance, so this applies + * uniformly to any MAP/ARRAY column. No-op when no such child columns exist. */ private void dropStructChildrenFor(String parentName, CalcitePlanContext context) { String prefix = parentName + "."; diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java index 8b90f3f2a42..2d091d139de 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java @@ -202,32 +202,31 @@ public void testLiteralDottedColumnSurvivesScalarParentOverride() throws IOExcep } @Test - public void testFunctionBuiltContainerOverridePreservesLiteralDottedColumn() - throws IOException { - // Reviewer's case (a) on PR #5726: `arr` here is a function-built ARRAY, not a mapped - // parent — it cannot have stale flattened leaves, so overriding it must not touch the - // user-created literal column `arr.x`. - JSONObject result = - executeQuery( - String.format( - "source=%s | eval arr = array(1,2) | eval `arr.x` = 5 | eval arr = array(3,4) |" - + " fields arr, `arr.x`", - COLLISION_INDEX)); - verifyDataRows(result, rows(new org.json.JSONArray(java.util.List.of(3, 4)), 5)); + public void testFunctionBuiltContainerOverrideShadowsDottedSubtree() { + // Reviewer scenario (a) on PR #5726: the row schema carries no parent-child provenance, so + // reassigning any container-typed column consistently shadows its dotted subtree, including + // literal dotted columns created in between. `arr.x` is unreachable after `arr` is rebuilt. + assertThrows( + ResponseException.class, + () -> + executeQuery( + String.format( + "source=%s | eval arr = array(1,2) | eval `arr.x` = 5 | eval arr = array(3,4)" + + " | fields arr, `arr.x`", + COLLISION_INDEX))); } @Test - public void testRepeatedSpathPreservesInterveningLiteralDottedColumn() throws IOException { - // Reviewer's case (b) on PR #5726: the second spath overrides `data`, which at that point - // is the extraction MAP from the first spath (function-built, no stale mapped leaves). - // The literal column `data.custom` created in between must survive. - JSONObject result = - executeQuery( - String.format( - "source=%s | spath input=body output=data | eval `data.custom` = 'kept' | spath" - + " input=body output=data | fields data, `data.custom`", - COLLISION_INDEX)); - verifyDataRows( - result, rows(ImmutableMap.of("level", "ERROR", "msg", "from json"), "kept")); + public void testRepeatedSpathShadowsInterveningLiteralDottedColumn() { + // Reviewer scenario (b) on PR #5726: same consistent rule for a rebuilt spath output. The + // second spath overrides the MAP column `data` and sheds `data.custom` created in between. + assertThrows( + ResponseException.class, + () -> + executeQuery( + String.format( + "source=%s | spath input=body output=data | eval `data.custom` = 'kept' |" + + " spath input=body output=data | fields data, `data.custom`", + COLLISION_INDEX))); } }