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)); + } +}