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