From afc25a9f9a0e17409812e699433911a0738afcb9 Mon Sep 17 00:00:00 2001 From: Kai Huang Date: Wed, 9 Sep 2026 11:26:59 -0700 Subject: [PATCH] Resolve an object-vs-scalar mapping conflict to the scalar (#5752) When a wildcard spans indices that disagree on whether a path is an object or a scalar -- as happens when a mapping changes at a rollover boundary -- the pair matches no merge rule: DeepMergeRule needs both sides to share an ExprCoreType and TextKeywordConflictRule only matches the text family. It therefore falls through to LatestRule, so the winner is whichever index is merged last. That order comes from the mapping map, which OpenSearchNodeClient builds with Collectors.toUnmodifiableMap. The JDK randomizes iteration order of its immutable maps per JVM (ImmutableCollections.SALT), so the resolved type differs between nodes and changes when a node restarts. Measured on six freshly started clusters with one keyword index and one object index, the same query resolved the path as an object three times and as a string three times. When it lands on the object side, every value aggregates to null and `timechart ... by ` fails (#5750). Add ObjectScalarConflictRule, ahead of LatestRule, resolving such a path to the scalar side. The scalar is the only side holding a value that can be grouped, sorted or charted, and it keeps doc-values pushdown available; resolving to text instead (what the text/keyword rule does for its case) would force _source retrieval and a full scan. Documents from the indices that map the path as an object have no scalar there, so they aggregate into the missing bucket, the same as a field absent from an index. The predicate decides on the mapping type rather than the ExprCoreType, because text, match_only_text, geo_point and binary all resolve to UNKNOWN while still holding one value per document -- keying on the core type would have left the most common conflict, text vs object, on the coin flip. Two consequences worth noting for review: - The object's sub-fields go with it, so `path.sub` over the wildcard now fails with "field not found" where it previously worked whenever the object side happened to win. Retaining the sub-fields was tried first and is worse: the row cannot hold both a scalar and a subtree at one path, so the retained column resolved but always read null. Querying the object-mapped index directly still returns the sub-fields. - Aggregating over the path puts the object side's documents in the missing bucket rather than excluding them, so counts still add up. Signed-off-by: Kai Huang --- .../test/ppl/object_scalar_conflict.yml | 176 ++++++++++++++++++ .../util/MergeRules/MergeRuleHelper.java | 5 +- .../MergeRules/ObjectScalarConflictRule.java | 68 +++++++ .../ObjectScalarConflictRuleTest.java | 106 +++++++++++ 4 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/object_scalar_conflict.yml create mode 100644 opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRule.java create mode 100644 opensearch/src/test/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRuleTest.java diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/object_scalar_conflict.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/object_scalar_conflict.yml new file mode 100644 index 00000000000..807cc8c6ae8 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/ppl/object_scalar_conflict.yml @@ -0,0 +1,176 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + # `labels.zone` is a keyword in one index and an object in the other, as happens when a + # mapping changes at a rollover boundary. The wildcard below spans both, so the merge has to + # pick one type; it must pick the same one on every node and every request. + - do: + indices.create: + index: conflict_scalar + body: + mappings: + properties: + "@timestamp": + type: date + labels: + properties: + zone: + type: keyword + - do: + indices.create: + index: conflict_object + body: + mappings: + properties: + "@timestamp": + type: date + labels: + properties: + zone: + properties: + name: + type: keyword + - do: + indices.create: + index: zone_text_only + body: + mappings: + properties: + "@timestamp": + type: date + labels: + properties: + zone: + type: text + - do: + bulk: + index: zone_text_only + refresh: true + body: + - '{"index": {}}' + - '{"@timestamp": "2026-01-01T00:03:00Z", "labels": {"zone": "z4"}}' + - do: + bulk: + index: conflict_scalar + refresh: true + body: + - '{"index": {}}' + - '{"@timestamp": "2026-01-01T00:01:00Z", "labels": {"zone": "z1"}}' + - '{"index": {}}' + - '{"@timestamp": "2026-01-01T00:01:30Z", "labels": {"zone": "z2"}}' + - do: + bulk: + index: conflict_object + refresh: true + body: + - '{"index": {}}' + - '{"@timestamp": "2026-01-01T00:02:00Z", "labels": {"zone": {"name": "z3"}}}' + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + - do: + indices.delete: + index: conflict_scalar + ignore: 404 + - do: + indices.delete: + index: conflict_object + ignore: 404 + - do: + indices.delete: + index: zone_text_only + ignore: 404 + +--- +"a path that is an object in one index and a scalar in another resolves to the scalar": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=conflict_* | sort `@timestamp` | fields `labels.zone`" + - match: {"schema": [{"name": "labels.zone", "type": "string"}]} + - match: {"total": 3} + - match: {"datarows.0": ["z1"]} + - match: {"datarows.1": ["z2"]} + +--- +"grouping by the conflicting path returns the scalar values, with the object side in the missing bucket": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=conflict_* | stats count() by `labels.zone` | sort `labels.zone`" + - match: {"schema": [{"name": "count()", "type": "bigint"}, {"name": "labels.zone", "type": "string"}]} + - match: {"datarows": [[1, null], [1, "z1"], [1, "z2"]]} + +--- +"timechart can split by the conflicting path": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=conflict_* | timechart span=1m count() by `labels.zone`" + - match: {"schema": [{"name": "@timestamp", "type": "timestamp"}, {"name": "labels.zone", "type": "string"}, {"name": "count()", "type": "bigint"}]} + - match: {"total": 3} + +--- +"the sub-fields of the object side are dropped with it": + - skip: + features: + - headers + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=conflict_* | fields `labels.zone.name`" + - match: {"$body": "/Field\\s+.labels.zone.name.\\s+not\\s+found/"} + +--- +"the sub-fields are still available when the object-mapped index is queried directly": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=conflict_object | fields `labels.zone.name`" + - match: {"schema": [{"name": "labels.zone.name", "type": "string"}]} + - match: {"datarows": [["z3"]]} + +--- +"a text mapping also wins the path against an object": + - skip: + features: + - headers + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=zone_text_only,conflict_object | fields `labels.zone`" + - match: {"schema": [{"name": "labels.zone", "type": "string"}]} + - match: {"total": 2} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/MergeRuleHelper.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/MergeRuleHelper.java index 6cc6f1803a7..614f2f97951 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/MergeRuleHelper.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/MergeRuleHelper.java @@ -12,7 +12,10 @@ public class MergeRuleHelper { private static final List RULES = List.of( - new DeepMergeRule(), new TextKeywordConflictRule(), new LatestRule() // must come last + new DeepMergeRule(), + new TextKeywordConflictRule(), + new ObjectScalarConflictRule(), + new LatestRule() // must come last ); public static MergeRule selectRule(OpenSearchDataType source, OpenSearchDataType target) { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRule.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRule.java new file mode 100644 index 00000000000..455bcd9565b --- /dev/null +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRule.java @@ -0,0 +1,68 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.opensearch.util.MergeRules; + +import java.util.Map; +import org.opensearch.sql.data.type.ExprCoreType; +import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; +import org.opensearch.sql.opensearch.data.type.OpenSearchDataType.MappingType; + +/** + * Merge rule for object/scalar type conflicts across indices. When a path is an object (or nested) + * in one index and a scalar in another -- typically after a mapping change at a rollover boundary + * -- the path resolves to the scalar type. + * + *

Without this rule the pair matches no other rule and falls through to {@link LatestRule}, so + * the winner is whichever index is merged last. That order comes from the mapping map built with + * {@code Collectors.toUnmodifiableMap}, whose iteration order the JDK randomizes per JVM, making + * the resolved type differ between nodes and change across restarts. + * + *

The scalar side wins because it is the only side with a value that can be grouped, sorted or + * charted, and it keeps doc-values pushdown available. Documents from the indices that map the path + * as an object have no scalar there, so they aggregate into the missing bucket. The object's + * sub-fields are dropped along with it: the row cannot hold both a scalar and a subtree at one + * path, so keeping them would resolve `path.sub` to a column that always reads null. Failing such a + * query with "field not found" is the honest outcome; querying the object-mapped index directly + * still returns the sub-fields. + * + *

See GitHub issue #5752. + */ +public class ObjectScalarConflictRule implements MergeRule { + + @Override + public boolean isMatch(OpenSearchDataType source, OpenSearchDataType target) { + if (source == null || target == null) { + return false; + } + return (isContainer(source) && isScalar(target)) || (isScalar(source) && isContainer(target)); + } + + @Override + public void mergeInto( + String key, OpenSearchDataType source, Map target) { + OpenSearchDataType scalar = isContainer(source) ? target.get(key) : source; + target.put(key, scalar); + } + + /** An object or nested type, i.e. one whose value is a subtree rather than a single value. */ + private static boolean isContainer(OpenSearchDataType type) { + ExprCoreType coreType = type.getExprCoreType(); + return coreType == ExprCoreType.STRUCT || coreType == ExprCoreType.ARRAY; + } + + /** + * A single-valued type. Decided on the mapping type rather than the {@link ExprCoreType}, because + * text, match_only_text, geo_point and binary all resolve to {@link ExprCoreType#UNKNOWN} while + * still holding one value per document. An alias only redirects to another path, so it is left to + * the other rules. + */ + private static boolean isScalar(OpenSearchDataType type) { + MappingType mappingType = type.getMappingType(); + return !isContainer(type) + && mappingType != MappingType.Alias + && mappingType != MappingType.Invalid; + } +} diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRuleTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRuleTest.java new file mode 100644 index 00000000000..fbdca8f0b74 --- /dev/null +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/util/MergeRules/ObjectScalarConflictRuleTest.java @@ -0,0 +1,106 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.opensearch.util.MergeRules; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; +import org.opensearch.sql.opensearch.data.type.OpenSearchDataType.MappingType; +import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; + +class ObjectScalarConflictRuleTest { + + private final ObjectScalarConflictRule rule = new ObjectScalarConflictRule(); + + /** An object type mapping the given keyword sub-fields. */ + private static OpenSearchDataType object(String... subFields) { + Map raw = new LinkedHashMap<>(); + for (String subField : subFields) { + raw.put(subField, Map.of("type", "keyword")); + } + return OpenSearchDataType.of(MappingType.Object, Map.of("properties", raw)); + } + + @Test + void matchesObjectAgainstScalarInBothDirections() { + OpenSearchDataType keyword = OpenSearchDataType.of(MappingType.Keyword); + assertTrue(rule.isMatch(object("name"), keyword)); + assertTrue(rule.isMatch(keyword, object("name"))); + } + + @Test + void matchesNestedAgainstScalar() { + OpenSearchDataType nested = OpenSearchDataType.of(MappingType.Nested); + OpenSearchDataType keyword = OpenSearchDataType.of(MappingType.Keyword); + assertTrue(rule.isMatch(nested, keyword)); + assertTrue(rule.isMatch(keyword, nested)); + } + + @Test + void doesNotMatchWhenNeitherSideIsAContainer() { + assertFalse( + rule.isMatch( + OpenSearchDataType.of(MappingType.Keyword), + OpenSearchDataType.of(MappingType.Integer))); + assertFalse(rule.isMatch(OpenSearchTextType.of(), OpenSearchDataType.of(MappingType.Keyword))); + } + + @Test + void doesNotMatchWhenBothSidesAreContainers() { + assertFalse(rule.isMatch(object("name"), object("uid"))); + assertFalse( + rule.isMatch( + OpenSearchDataType.of(MappingType.Nested), OpenSearchDataType.of(MappingType.Nested))); + } + + @Test + void matchesTextAgainstObject() { + assertTrue(rule.isMatch(object("name"), OpenSearchTextType.of())); + assertTrue(rule.isMatch(OpenSearchTextType.of(), object("name"))); + } + + @Test + void doesNotMatchNullSide() { + assertFalse(rule.isMatch(object("name"), null)); + assertFalse(rule.isMatch(null, OpenSearchDataType.of(MappingType.Keyword))); + } + + /** The scalar wins the path whichever side of the merge it arrives on. */ + @Test + void resolvesToTheScalarRegardlessOfMergeOrder() { + Map objectFirst = new HashMap<>(); + objectFirst.put("zone", object("name")); + rule.mergeInto("zone", OpenSearchDataType.of(MappingType.Keyword), objectFirst); + assertEquals(MappingType.Keyword, objectFirst.get("zone").getMappingType()); + + Map scalarFirst = new HashMap<>(); + scalarFirst.put("zone", OpenSearchDataType.of(MappingType.Keyword)); + rule.mergeInto("zone", object("name"), scalarFirst); + assertEquals(MappingType.Keyword, scalarFirst.get("zone").getMappingType()); + } + + /** + * The object's sub-fields go with it. A row cannot hold both a scalar and a subtree at one path, + * so a retained `path.sub` column would always read null; failing it as "field not found" is the + * honest outcome. + */ + @Test + void dropsTheContainerSubFields() { + Map target = new HashMap<>(); + target.put("zone", object("name", "uid")); + rule.mergeInto("zone", OpenSearchDataType.of(MappingType.Keyword), target); + + OpenSearchDataType merged = target.get("zone"); + assertEquals(MappingType.Keyword, merged.getMappingType()); + assertTrue(merged.getProperties().isEmpty()); + } +}