-
Notifications
You must be signed in to change notification settings - Fork 225
[BugFix] Shadow stale mapped leaves when an override replaces an object parent (#5718) #5726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Ystk-hsn
wants to merge
3
commits into
opensearch-project:main
Choose a base branch
from
Ystk-hsn:fix/5718-spath-output-collision
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
232 changes: 232 additions & 0 deletions
232
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCollisionIT.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| /* | ||
| * 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 <b>object</b> | ||
| * field. | ||
| * | ||
| * <p>Expected semantics (issue #5718, preferred option): overriding an object parent shadows the | ||
| * <b>entire</b> {@code <name>.*} subtree. After {@code spath input=body output=log}, every {@code | ||
| * log.<key>} 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)); | ||
| } | ||
|
|
||
| @Test | ||
| 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 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))); | ||
| } | ||
| } | ||
89 changes: 89 additions & 0 deletions
89
integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5718.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.