From 2ad9c97a31c148da88a4cdb8d572cf81664a829f Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 10:33:21 +0800 Subject: [PATCH 1/6] [BugFix] Fix PPL nested field access inside disabled object fields (#4906) When an index mapping declares an object with "enabled": false (or any object without indexed properties), inner fields have no mapping entry. The data layer previously stringified the unmapped nested value, so querying `fields log.c.d` returned null with type "undefined". Fix: - `OpenSearchExprValueFactory.parse` recurses into the unmapped object via a new `parseUnmappedStruct` helper instead of stringifying it. - `parseStruct` additionally exposes every descendant of an unmapped child under a dotted-path key at the current level, so the Calcite ITEM operator (which treats a tuple as a flat map) can resolve `ITEM(log, "c.d")` with a single lookup. - New `Content.isObject()` default method detects object/map-like content for both JsonNode and plain Java Map sources. Covered by a new integration test (`CalciteDisabledObjectFieldIT`, also added to `CalciteNoPushdownIT`), new unit tests in `OpenSearchExprValueFactoryTest`, and YAML REST test `issues/4906.yml`. Signed-off-by: Heng Qian --- .../sql/calcite/CalciteNoPushdownIT.java | 1 + .../remote/CalciteDisabledObjectFieldIT.java | 83 ++++++++++++++++ .../rest-api-spec/test/issues/4906.yml | 50 ++++++++++ .../sql/opensearch/data/utils/Content.java | 12 +++ .../value/OpenSearchExprValueFactory.java | 95 ++++++++++++++++++- .../value/OpenSearchExprValueFactoryTest.java | 72 ++++++++++++++ 6 files changed, 309 insertions(+), 4 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4906.yml diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java index 29c2ffd582f..aa76b87a5c8 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java @@ -32,6 +32,7 @@ CalciteDataTypeIT.class, CalciteDateTimeComparisonIT.class, CalciteDateTimeFunctionIT.class, + CalciteDisabledObjectFieldIT.class, CalciteDateTimeImplementationIT.class, CalciteDedupCommandIT.class, CalciteDescribeCommandIT.class, diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java new file mode 100644 index 00000000000..ff6401e17d7 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java @@ -0,0 +1,83 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.calcite.remote; + +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; +import static org.opensearch.sql.util.TestUtils.createIndexByRestClient; +import static org.opensearch.sql.util.TestUtils.isIndexExist; +import static org.opensearch.sql.util.TestUtils.performRequest; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; +import org.opensearch.client.Request; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +/** + * Integration tests for querying inner fields of an object that is declared with {@code "enabled": + * false} in the index mapping. Such objects are stored in {@code _source} but are not indexed, so + * the plugin cannot learn the shape of the object from the mapping. See GitHub issue #4906. + */ +public class CalciteDisabledObjectFieldIT extends PPLIntegTestCase { + + private static final String DISABLED_OBJECT_INDEX = "test_disabled_object_4906"; + + @Override + public void init() throws Exception { + super.init(); + enableCalcite(); + createTestIndex(); + } + + private void createTestIndex() throws IOException { + if (!isIndexExist(client(), DISABLED_OBJECT_INDEX)) { + String mapping = + "{\"mappings\":{\"properties\":{" + + "\"log\":{\"type\":\"object\",\"enabled\":false}" + + "}}}"; + createIndexByRestClient(client(), DISABLED_OBJECT_INDEX, mapping); + Request bulkReq = new Request("POST", "/" + DISABLED_OBJECT_INDEX + "/_bulk?refresh=true"); + bulkReq.setJsonEntity( + "{\"index\":{\"_id\":\"1\"}}\n" + "{\"log\":{\"a\":1,\"c\":{\"d\":2}}}\n"); + performRequest(client(), bulkReq); + } + } + + @Test + public void testSelectNestedFieldFromDisabledObject() throws IOException { + JSONObject result = + executeQuery(String.format("source=%s | fields log.c.d", DISABLED_OBJECT_INDEX)); + // Schema is undefined because disabled object fields have no field mapping. + // The important check is that the value is extracted correctly from _source. + verifyDataRows(result, rows(2)); + } + + @Test + public void testSelectTopLevelFieldFromDisabledObject() throws IOException { + JSONObject result = + executeQuery(String.format("source=%s | fields log.a", DISABLED_OBJECT_INDEX)); + // Schema is undefined because disabled object fields have no field mapping. + // The important check is that the value is extracted correctly from _source. + verifyDataRows(result, rows(1)); + } + + @Test + public void testSelectEntireDisabledObject() throws IOException { + JSONObject result = + executeQuery(String.format("source=%s | fields log", DISABLED_OBJECT_INDEX)); + verifySchema(result, schema("log", "struct")); + } + + @Test + public void testSelectIntermediateFieldFromDisabledObject() throws IOException { + JSONObject result = + executeQuery(String.format("source=%s | fields log.c", DISABLED_OBJECT_INDEX)); + verifySchema(result, schema("log.c", "struct")); + } +} diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4906.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4906.yml new file mode 100644 index 00000000000..da650014d97 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4906.yml @@ -0,0 +1,50 @@ +setup: + - do: + indices.create: + index: test + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + log: + type: object + enabled: false + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + +--- +"Access nested field inside a disabled object": + - skip: + features: + - headers + - allowed_warnings + - do: + bulk: + index: test + refresh: true + body: + - '{"index": {}}' + - '{"log": {"a": 1, "c": {"d": 2}}}' + - do: + allowed_warnings: + - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled' + headers: + Content-Type: 'application/json' + ppl: + body: + query: 'source=test | fields log.c.d' + - match: {"total": 1} + - match: {"datarows": [[2]]} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java index d2fa68dac06..96dc2658a27 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java @@ -53,6 +53,18 @@ public interface Content { /** Is array value. */ boolean isArray(); + /** Is an object / map-like value. */ + default boolean isObject() { + Object raw = objectValue(); + if (raw instanceof Map) { + return true; + } + if (raw instanceof com.fasterxml.jackson.databind.JsonNode node) { + return node.isObject(); + } + return false; + } + /** Get integer value. */ Integer intValue(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index d772b3e603b..90d2e355901 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -203,6 +203,12 @@ private ExprValue parse( // is resolved if (fieldType.isEmpty() || fieldType.get().equals(OpenSearchDataType.of(ExprCoreType.UNDEFINED))) { + // If the content is an object (e.g. inside a disabled object field, see issue #4906), + // recurse into it as a struct so downstream path access (log.c.d) works. Without this, + // parseContent would fall through to stringify the object and nested keys would be lost. + if (content.isObject()) { + return parseUnmappedStruct(content, field, supportArrays); + } return parseContent(content); } @@ -362,6 +368,62 @@ private static ExprValue createOpenSearchDateType( return new ExprTimestampValue((Instant) value.objectValue()); } + /** + * Parse an unmapped object (e.g. an object declared with {@code "enabled": false} or generated by + * dynamic mapping with no known schema — see issue #4906). Produces an {@link ExprTupleValue} + * that holds each descendant value under both its nested key and the dotted-path key, so later + * {@code ITEM} lookups (which treat the tuple as a flat map) can retrieve values at any depth + * with a single key. + * + *

Example: an input of {@code {"a": 1, "c": {"d": 2}}} produces a tuple with entries {@code + * a=1}, {@code c=}, {@code c.d=2} where the {@code c} entry is itself expanded the same + * way. + * + * @param content Content to parse. + * @param prefix Prefix for level of object depth to parse. + * @param supportArrays Parsing the whole array if array is type nested. + * @return Value parsed from content. + */ + private ExprValue parseUnmappedStruct(Content content, String prefix, boolean supportArrays) { + ExprTupleValue result = ExprTupleValue.empty(); + content + .map() + .forEachRemaining( + entry -> { + String fieldKey = entry.getKey(); + if (isFieldNameMalformed(fieldKey)) { + result.tupleValue().put(fieldKey, ExprNullValue.of()); + return; + } + Content childContent = entry.getValue(); + String fullFieldPath = makeField(prefix, fieldKey); + // Always recurse on child values using the unmapped-struct parser so nested maps + // are preserved. Single ITEM lookups will still succeed because we also expose + // descendants under their dotted-path keys below. + ExprValue childValue; + if (childContent.isNull()) { + childValue = ExprNullValue.of(); + } else if (childContent.isObject()) { + childValue = parseUnmappedStruct(childContent, fullFieldPath, supportArrays); + } else if (childContent.isArray()) { + childValue = parseContent(childContent); + } else { + childValue = parseContent(childContent); + } + result.tupleValue().put(fieldKey, childValue); + // Additionally expose every descendant at the current level using dotted-path keys + // so ITEM(parent, "c.d") can resolve to a scalar leaf value. Intermediate dotted + // paths (e.g. "c" mapping to a tuple) are already provided by the direct child entry. + if (childValue instanceof ExprTupleValue childTuple) { + for (Map.Entry descendant : childTuple.tupleValue().entrySet()) { + String descendantKey = fieldKey + "." + descendant.getKey(); + result.tupleValue().putIfAbsent(descendantKey, descendant.getValue()); + } + } + }); + return result; + } + /** * Parse struct content. * @@ -383,15 +445,40 @@ private ExprValue parseStruct(Content content, String prefix, boolean supportArr if (isFieldNameMalformed(fieldKey)) { result.tupleValue().put(fieldKey, ExprNullValue.of()); } else { - populateValueRecursive( - result, - new JsonPath(fieldKey), - parse(entry.getValue(), fullFieldPath, type(fullFieldPath), supportArrays)); + Optional childType = type(fullFieldPath); + ExprValue childValue = + parse(entry.getValue(), fullFieldPath, childType, supportArrays); + populateValueRecursive(result, new JsonPath(fieldKey), childValue); + // If the child's type is unmapped (e.g. inside a disabled object — see #4906), + // also expose every descendant under its dotted-path key so single-key ITEM + // lookups at this level resolve nested paths like log.c.d. + if (isUnmappedType(childType) && childValue instanceof ExprTupleValue) { + exposeDescendantsAsDottedKeys(result, fieldKey, (ExprTupleValue) childValue); + } } }); return result; } + private static boolean isUnmappedType(Optional fieldType) { + return fieldType.isEmpty() + || fieldType.get().equals(OpenSearchDataType.of(ExprCoreType.UNDEFINED)); + } + + /** + * Add every descendant entry already present in {@code childTuple} to {@code result} under a + * dotted-path key rooted at {@code fieldKey}. The child tuple is assumed to have been produced by + * {@link #parseUnmappedStruct(Content, String, boolean)} which already expanded nested tuples + * into dotted-path entries at each level. Existing keys are preserved. + */ + private static void exposeDescendantsAsDottedKeys( + ExprTupleValue result, String fieldKey, ExprTupleValue childTuple) { + for (Map.Entry descendant : childTuple.tupleValue().entrySet()) { + String descendantKey = fieldKey + "." + descendant.getKey(); + result.tupleValue().putIfAbsent(descendantKey, descendant.getValue()); + } + } + /** * Check if a field name is malformed and cannot be processed by JsonPath. * diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 031b9243f38..913442b3fba 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -1180,6 +1180,78 @@ public void constructStructWithMalformedAndValidFields_preservesValidFields() { assertEquals(stringValue("value"), structValue.get("good")); } + // ==================== Disabled Object Field Tests ==================== + // Tests for issue #4906: PPL query returns null when accessing nested keys of an object + // declared with "enabled": false. Such objects are stored in _source but not indexed, so the + // plugin cannot learn their inner schema. The factory must parse the raw _source recursively + // AND also expose each descendant under a dotted-path key so the Calcite ITEM operator — which + // treats a tuple as a flat map — can resolve log.c.d with a single lookup. + + private OpenSearchExprValueFactory disabledObjectFactory() { + // "log" is mapped as an object with no properties (equivalent to {type:object, enabled:false}). + return new OpenSearchExprValueFactory( + Map.of("log", OpenSearchDataType.of(OpenSearchDataType.MappingType.Object)), true); + } + + @Test + public void disabledObject_topLevelScalarChildRetainsType() { + Map tuple = + disabledObjectFactory() + .construct("{\"log\":{\"a\":1,\"c\":{\"d\":2}}}", false) + .tupleValue(); + ExprValue log = tuple.get("log"); + assertEquals(integerValue(1), log.tupleValue().get("a")); + } + + @Test + public void disabledObject_intermediateChildIsParsedAsTuple() { + Map tuple = + disabledObjectFactory() + .construct("{\"log\":{\"a\":1,\"c\":{\"d\":2}}}", false) + .tupleValue(); + ExprValue logC = tuple.get("log").tupleValue().get("c"); + assertTrue(logC instanceof ExprTupleValue); + assertEquals(integerValue(2), logC.tupleValue().get("d")); + } + + @Test + public void disabledObject_leafExposedUnderDottedKeyForItemLookup() { + // The core fix: ITEM(log, "c.d") must resolve to 2. This requires the "log" tuple to carry + // the flat dotted-path key "c.d" in addition to the nested "c" -> tuple{d:2}. + Map tuple = + disabledObjectFactory() + .construct("{\"log\":{\"a\":1,\"c\":{\"d\":2}}}", false) + .tupleValue(); + ExprValue log = tuple.get("log"); + assertEquals(integerValue(2), log.tupleValue().get("c.d")); + } + + @Test + public void disabledObject_deeplyNestedLeafExposedAtEveryAncestor() { + // Verify that deeper nesting {c:{d:{e:3}}} works with single-key ITEM at any level. + Map tuple = + disabledObjectFactory() + .construct("{\"log\":{\"c\":{\"d\":{\"e\":3}}}}", false) + .tupleValue(); + ExprValue log = tuple.get("log"); + assertEquals(integerValue(3), log.tupleValue().get("c.d.e")); + ExprValue logC = log.tupleValue().get("c"); + assertEquals(integerValue(3), logC.tupleValue().get("d.e")); + } + + @Test + public void disabledObject_mappedSiblingsAreUnaffected() { + // With a normal mapped struct (structV.id, structV.state), the factory should NOT inject flat + // dotted keys at the top level. Only the existing behaviour is preserved. + Map tuple = tupleValue("{\"structV\":{\"id\":1,\"state\":\"WA\"}}"); + ExprValue structV = tuple.get("structV"); + assertEquals(integerValue(1), structV.tupleValue().get("id")); + assertEquals(stringValue("WA"), structV.tupleValue().get("state")); + // No accidental flat-key injection on mapped structs. + assertFalse(structV.tupleValue().containsKey("structV.id")); + assertFalse(tuple.containsKey("structV.id")); + } + @EqualsAndHashCode(callSuper = false) @ToString private static class TestType extends OpenSearchDataType { From df4e4acecda1533ccf19dbf8014993ecddc64d34 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 13:37:10 +0800 Subject: [PATCH 2/6] Address review feedback: clean up Content abstraction and redundant branch - Make Content.isObject() abstract so implementations override it directly, removing the Jackson leak from the default method. - Collapse the redundant isArray()/else branches in parseUnmappedStruct into a single else clause with a comment. Signed-off-by: Heng Qian --- .../opensearch/sql/opensearch/data/utils/Content.java | 11 +---------- .../sql/opensearch/data/utils/ObjectContent.java | 5 +++++ .../opensearch/data/utils/OpenSearchJsonContent.java | 5 +++++ .../data/value/OpenSearchExprValueFactory.java | 3 +-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java index 96dc2658a27..b795d95070b 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/Content.java @@ -54,16 +54,7 @@ public interface Content { boolean isArray(); /** Is an object / map-like value. */ - default boolean isObject() { - Object raw = objectValue(); - if (raw instanceof Map) { - return true; - } - if (raw instanceof com.fasterxml.jackson.databind.JsonNode node) { - return node.isObject(); - } - return false; - } + boolean isObject(); /** Get integer value. */ Integer intValue(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/ObjectContent.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/ObjectContent.java index b409640c993..e657513246d 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/ObjectContent.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/ObjectContent.java @@ -144,6 +144,11 @@ public boolean isArray() { return value instanceof ArrayNode || value instanceof List; } + @Override + public boolean isObject() { + return value instanceof Map; + } + @Override public boolean isString() { return value instanceof String; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java index 93c2c6b1584..7c9adedf43f 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java @@ -140,6 +140,11 @@ public boolean isArray() { return value().isArray(); } + @Override + public boolean isObject() { + return value != null && value.isObject(); + } + @Override public Object objectValue() { return value(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index 90d2e355901..e1937f6208b 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -405,9 +405,8 @@ private ExprValue parseUnmappedStruct(Content content, String prefix, boolean su childValue = ExprNullValue.of(); } else if (childContent.isObject()) { childValue = parseUnmappedStruct(childContent, fullFieldPath, supportArrays); - } else if (childContent.isArray()) { - childValue = parseContent(childContent); } else { + // Scalars and arrays both go through parseContent, which handles them correctly. childValue = parseContent(childContent); } result.tupleValue().put(fieldKey, childValue); From dd1e295ba5242accfbd98679aa4406893dc41561 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 13:53:02 +0800 Subject: [PATCH 3/6] [BugFix] Refactor parseContent/parseStruct per review (#4906) Address two inline review comments on PR #5390: - Line 209: the object handling in parse() is now inside parseContent, which is the single place where content-shape dispatch happens for unmapped/UNDEFINED field types. parse() delegates directly to parseContent for the unmapped branch. parseUnmappedStruct's child recursion is also simplified to go through parseContent (handles nulls/scalars/objects uniformly). - Line 454: the per-child body of parseStruct is extracted into parseStructEntry. The main parseStruct body becomes a trivial forEachRemaining; the dotted-key exposure logic moves behind a helper that sits next to parse() in the call graph and is invoked once per child entry. Signed-off-by: Heng Qian --- .../value/OpenSearchExprValueFactory.java | 99 ++++++++++--------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index e1937f6208b..341aeca5799 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -203,13 +203,7 @@ private ExprValue parse( // is resolved if (fieldType.isEmpty() || fieldType.get().equals(OpenSearchDataType.of(ExprCoreType.UNDEFINED))) { - // If the content is an object (e.g. inside a disabled object field, see issue #4906), - // recurse into it as a struct so downstream path access (log.c.d) works. Without this, - // parseContent would fall through to stringify the object and nested keys would be lost. - if (content.isObject()) { - return parseUnmappedStruct(content, field, supportArrays); - } - return parseContent(content); + return parseContent(content, field, supportArrays); } final ExprType type = fieldType.get(); @@ -233,7 +227,15 @@ private ExprValue parse( } } - private ExprValue parseContent(Content content) { + /** + * Parse {@link Content} into an {@link ExprValue} when the declared field type is unknown (either + * absent from the mapping or {@code UNDEFINED}). Dispatches purely on the shape of the content. + * + *

Objects are recursed into as unmapped structs (see issue #4906). Without this, an object + * inside a disabled-object field would be stringified and nested keys like {@code log.c.d} would + * be lost. + */ + private ExprValue parseContent(Content content, String field, boolean supportArrays) { if (content.isNumber()) { if (content.isInt()) { return new ExprIntegerValue(content.intValue()); @@ -257,6 +259,8 @@ private ExprValue parseContent(Content content) { return ExprBooleanValue.of(content.booleanValue()); } else if (content.isNull()) { return ExprNullValue.of(); + } else if (content.isObject()) { + return parseUnmappedStruct(content, field, supportArrays); } // Default case, treat as a string value return new ExprStringValue(content.objectValue().toString()); @@ -397,27 +401,16 @@ private ExprValue parseUnmappedStruct(Content content, String prefix, boolean su } Content childContent = entry.getValue(); String fullFieldPath = makeField(prefix, fieldKey); - // Always recurse on child values using the unmapped-struct parser so nested maps - // are preserved. Single ITEM lookups will still succeed because we also expose - // descendants under their dotted-path keys below. - ExprValue childValue; - if (childContent.isNull()) { - childValue = ExprNullValue.of(); - } else if (childContent.isObject()) { - childValue = parseUnmappedStruct(childContent, fullFieldPath, supportArrays); - } else { - // Scalars and arrays both go through parseContent, which handles them correctly. - childValue = parseContent(childContent); - } + // Recurse via parseContent, which handles nulls, scalars, and objects (by + // recursing back into this method). Single ITEM lookups still succeed because + // we also expose descendants under their dotted-path keys below. + ExprValue childValue = parseContent(childContent, fullFieldPath, supportArrays); result.tupleValue().put(fieldKey, childValue); // Additionally expose every descendant at the current level using dotted-path keys // so ITEM(parent, "c.d") can resolve to a scalar leaf value. Intermediate dotted // paths (e.g. "c" mapping to a tuple) are already provided by the direct child entry. if (childValue instanceof ExprTupleValue childTuple) { - for (Map.Entry descendant : childTuple.tupleValue().entrySet()) { - String descendantKey = fieldKey + "." + descendant.getKey(); - result.tupleValue().putIfAbsent(descendantKey, descendant.getValue()); - } + exposeDescendantsAsDottedKeys(result, fieldKey, childTuple); } }); return result; @@ -433,32 +426,44 @@ private ExprValue parseUnmappedStruct(Content content, String prefix, boolean su */ private ExprValue parseStruct(Content content, String prefix, boolean supportArrays) { ExprTupleValue result = ExprTupleValue.empty(); - content - .map() - .forEachRemaining( - entry -> { - String fieldKey = entry.getKey(); - String fullFieldPath = makeField(prefix, fieldKey); - // Check for malformed field names before creating JsonPath. - // See isFieldNameMalformed() for details on what constitutes a malformed field name. - if (isFieldNameMalformed(fieldKey)) { - result.tupleValue().put(fieldKey, ExprNullValue.of()); - } else { - Optional childType = type(fullFieldPath); - ExprValue childValue = - parse(entry.getValue(), fullFieldPath, childType, supportArrays); - populateValueRecursive(result, new JsonPath(fieldKey), childValue); - // If the child's type is unmapped (e.g. inside a disabled object — see #4906), - // also expose every descendant under its dotted-path key so single-key ITEM - // lookups at this level resolve nested paths like log.c.d. - if (isUnmappedType(childType) && childValue instanceof ExprTupleValue) { - exposeDescendantsAsDottedKeys(result, fieldKey, (ExprTupleValue) childValue); - } - } - }); + content.map().forEachRemaining(entry -> parseStructEntry(result, prefix, entry, supportArrays)); return result; } + /** + * Parse a single entry of a struct and merge it into {@code result}. This is the per-child body + * of {@link #parseStruct}: it resolves the child's type, parses it via {@link #parse}, and + * inserts it into the parent tuple — additionally exposing the child's descendants under + * dotted-path keys when the child is unmapped (see issue #4906). + * + *

The dotted-key exposure is part of the parse contract for this child rather than something + * the outer loop needs to know about, which keeps {@link #parseStruct}'s body trivial. + */ + private void parseStructEntry( + ExprTupleValue result, + String prefix, + Map.Entry entry, + boolean supportArrays) { + String fieldKey = entry.getKey(); + // Check for malformed field names before creating JsonPath. + // See isFieldNameMalformed() for details on what constitutes a malformed field name. + if (isFieldNameMalformed(fieldKey)) { + result.tupleValue().put(fieldKey, ExprNullValue.of()); + return; + } + String fullFieldPath = makeField(prefix, fieldKey); + Optional childType = type(fullFieldPath); + ExprValue childValue = parse(entry.getValue(), fullFieldPath, childType, supportArrays); + populateValueRecursive(result, new JsonPath(fieldKey), childValue); + // If the child's type is unmapped (e.g. inside a disabled object — see #4906), also expose + // every descendant under its dotted-path key so single-key ITEM lookups at this level + // resolve nested paths like log.c.d. parseContent produces the flat entries inside the + // child tuple; this step just lifts them up one level with the fieldKey prefix. + if (isUnmappedType(childType) && childValue instanceof ExprTupleValue childTuple) { + exposeDescendantsAsDottedKeys(result, fieldKey, childTuple); + } + } + private static boolean isUnmappedType(Optional fieldType) { return fieldType.isEmpty() || fieldType.get().equals(OpenSearchDataType.of(ExprCoreType.UNDEFINED)); From ac4afb0c48370f3cb9808b6a8122bae1b933d889 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 14:14:08 +0800 Subject: [PATCH 4/6] [BugFix] Revert parseStructEntry extraction (#4906) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep `parseContent` unification (reviewer comment on line 209) but inline the per-child loop back into parseStruct — the extraction didn't fulfill the line-454 comment's intent (moving dotted-key exposure into parse) and a literal move would require an invasive signature change across all parse() call sites, so the cleaner path is to leave parseStruct owning that mutation since it's the only caller with access to the parent tuple. Signed-off-by: Heng Qian --- .../value/OpenSearchExprValueFactory.java | 58 ++++++++----------- 1 file changed, 23 insertions(+), 35 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index 341aeca5799..d6d2b0138af 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -426,44 +426,32 @@ private ExprValue parseUnmappedStruct(Content content, String prefix, boolean su */ private ExprValue parseStruct(Content content, String prefix, boolean supportArrays) { ExprTupleValue result = ExprTupleValue.empty(); - content.map().forEachRemaining(entry -> parseStructEntry(result, prefix, entry, supportArrays)); + content + .map() + .forEachRemaining( + entry -> { + String fieldKey = entry.getKey(); + String fullFieldPath = makeField(prefix, fieldKey); + // Check for malformed field names before creating JsonPath. + // See isFieldNameMalformed() for details on what constitutes a malformed field name. + if (isFieldNameMalformed(fieldKey)) { + result.tupleValue().put(fieldKey, ExprNullValue.of()); + } else { + Optional childType = type(fullFieldPath); + ExprValue childValue = + parse(entry.getValue(), fullFieldPath, childType, supportArrays); + populateValueRecursive(result, new JsonPath(fieldKey), childValue); + // If the child's type is unmapped (e.g. inside a disabled object — see #4906), + // also expose every descendant under its dotted-path key so single-key ITEM + // lookups at this level resolve nested paths like log.c.d. + if (isUnmappedType(childType) && childValue instanceof ExprTupleValue) { + exposeDescendantsAsDottedKeys(result, fieldKey, (ExprTupleValue) childValue); + } + } + }); return result; } - /** - * Parse a single entry of a struct and merge it into {@code result}. This is the per-child body - * of {@link #parseStruct}: it resolves the child's type, parses it via {@link #parse}, and - * inserts it into the parent tuple — additionally exposing the child's descendants under - * dotted-path keys when the child is unmapped (see issue #4906). - * - *

The dotted-key exposure is part of the parse contract for this child rather than something - * the outer loop needs to know about, which keeps {@link #parseStruct}'s body trivial. - */ - private void parseStructEntry( - ExprTupleValue result, - String prefix, - Map.Entry entry, - boolean supportArrays) { - String fieldKey = entry.getKey(); - // Check for malformed field names before creating JsonPath. - // See isFieldNameMalformed() for details on what constitutes a malformed field name. - if (isFieldNameMalformed(fieldKey)) { - result.tupleValue().put(fieldKey, ExprNullValue.of()); - return; - } - String fullFieldPath = makeField(prefix, fieldKey); - Optional childType = type(fullFieldPath); - ExprValue childValue = parse(entry.getValue(), fullFieldPath, childType, supportArrays); - populateValueRecursive(result, new JsonPath(fieldKey), childValue); - // If the child's type is unmapped (e.g. inside a disabled object — see #4906), also expose - // every descendant under its dotted-path key so single-key ITEM lookups at this level - // resolve nested paths like log.c.d. parseContent produces the flat entries inside the - // child tuple; this step just lifts them up one level with the fieldKey prefix. - if (isUnmappedType(childType) && childValue instanceof ExprTupleValue childTuple) { - exposeDescendantsAsDottedKeys(result, fieldKey, childTuple); - } - } - private static boolean isUnmappedType(Optional fieldType) { return fieldType.isEmpty() || fieldType.get().equals(OpenSearchDataType.of(ExprCoreType.UNDEFINED)); From 8aefdab9bfbe146b59bc4ff29e2524a3a2e169f3 Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 14:27:25 +0800 Subject: [PATCH 5/6] [BugFix] Address review comments on CalciteDisabledObjectFieldIT (#4906) Add verifySchema to leaf-field tests and verifyDataRows to struct-field tests as requested by reviewer. The schema for disabled object inner fields is inferred from the query path (int for leaf, struct for intermediate), so verifying both schema and rows now gives full coverage. Signed-off-by: Heng Qian --- .../sql/calcite/remote/CalciteDisabledObjectFieldIT.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java index ff6401e17d7..4bf31032b53 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisabledObjectFieldIT.java @@ -53,8 +53,7 @@ private void createTestIndex() throws IOException { public void testSelectNestedFieldFromDisabledObject() throws IOException { JSONObject result = executeQuery(String.format("source=%s | fields log.c.d", DISABLED_OBJECT_INDEX)); - // Schema is undefined because disabled object fields have no field mapping. - // The important check is that the value is extracted correctly from _source. + verifySchema(result, schema("log.c.d", "int")); verifyDataRows(result, rows(2)); } @@ -62,8 +61,7 @@ public void testSelectNestedFieldFromDisabledObject() throws IOException { public void testSelectTopLevelFieldFromDisabledObject() throws IOException { JSONObject result = executeQuery(String.format("source=%s | fields log.a", DISABLED_OBJECT_INDEX)); - // Schema is undefined because disabled object fields have no field mapping. - // The important check is that the value is extracted correctly from _source. + verifySchema(result, schema("log.a", "int")); verifyDataRows(result, rows(1)); } @@ -72,6 +70,7 @@ public void testSelectEntireDisabledObject() throws IOException { JSONObject result = executeQuery(String.format("source=%s | fields log", DISABLED_OBJECT_INDEX)); verifySchema(result, schema("log", "struct")); + verifyDataRows(result, rows(new JSONObject("{\"a\":1,\"c\":{\"d\":2},\"c.d\":2}"))); } @Test @@ -79,5 +78,6 @@ public void testSelectIntermediateFieldFromDisabledObject() throws IOException { JSONObject result = executeQuery(String.format("source=%s | fields log.c", DISABLED_OBJECT_INDEX)); verifySchema(result, schema("log.c", "struct")); + verifyDataRows(result, rows(new JSONObject("{\"d\":2}"))); } } From 34f9c299ab98bce6a7a64e7c9677bc12f493ecbf Mon Sep 17 00:00:00 2001 From: Heng Qian Date: Tue, 28 Apr 2026 16:12:24 +0800 Subject: [PATCH 6/6] [Enhancement] Use EnterWorktree for ppl-bugfix follow-up The 2B follow-up dispatch previously told the subagent to `cd first`, which led to `cd && cmd > file 2>&1 | tail` patterns that trip Claude Code's compound-cd-with-redirection safety check and require manual approval on every build/test invocation. Switch to calling EnterWorktree(path=...) as the subagent's first action so its session cwd is set to the existing worktree root. No more `cd` in Bash, no more approval prompts, matches the ergonomics of the 2A initial fix path which already relied on isolation: "worktree" to set cwd. Signed-off-by: Heng Qian --- .claude/commands/ppl-bugfix.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.claude/commands/ppl-bugfix.md b/.claude/commands/ppl-bugfix.md index f2ac6d6807d..7fefbe84568 100644 --- a/.claude/commands/ppl-bugfix.md +++ b/.claude/commands/ppl-bugfix.md @@ -95,16 +95,15 @@ for wt in .claude/worktrees/agent-*/; do done ``` -**If existing worktree found**: Do NOT use `isolation: "worktree"`. Pass the worktree path in the prompt so the subagent works there directly. +**If existing worktree found**: Do NOT use `isolation: "worktree"`. Instruct the subagent to call `EnterWorktree(path=...)` to switch cwd into the existing one. ``` Agent( mode: "", name: "bugfix-", description: "PPL bugfix # followup", - prompt: "cd first, then read .claude/harness/ppl-bugfix-followup.md and follow it. - PR: (), Issue: # - Working directory: " + prompt: "First, call EnterWorktree(path=\"\") to switch cwd. Then read .claude/harness/ppl-bugfix-followup.md and follow it. Do NOT use `cd` in Bash commands. + PR: (), Issue: #" ) ```