From 80b1f30e273a8bd0e6fe967141c158c2902dd336 Mon Sep 17 00:00:00 2001 From: Peng Huo Date: Thu, 10 Sep 2026 08:49:45 -0700 Subject: [PATCH] Fix PPL Calcite all-fields resolution for disabled objects Signed-off-by: Peng Huo --- .../sql/calcite/CalciteRelNodeVisitor.java | 271 +++++++++++++- .../org/opensearch/sql/storage/Table.java | 20 ++ .../sql/calcite/CalciteNoPushdownIT.java | 1 + .../remote/CalciteDisableObjectsIT.java | 331 ++++++++++++++++++ .../opensearch/sql/ppl/DisableObjectsIT.java | 69 ++++ .../data/type/OpenSearchDataType.java | 40 +++ .../opensearch/storage/OpenSearchIndex.java | 17 + .../data/type/OpenSearchDataTypeTest.java | 89 +++++ 8 files changed, 827 insertions(+), 11 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisableObjectsIT.java create mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/DisableObjectsIT.java 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 a2f0addbfb6..b03164f646e 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -28,8 +28,10 @@ import static org.opensearch.sql.utils.SystemIndexUtils.DATASOURCES_TABLE_NAME; import com.google.common.base.Strings; +import com.google.common.collect.HashMultiset; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.common.collect.Multiset; import com.google.common.collect.Streams; import java.math.BigDecimal; import java.util.ArrayList; @@ -56,11 +58,14 @@ import org.apache.calcite.rel.RelFieldCollation; import org.apache.calcite.rel.RelHomogeneousShuttle; import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.RelVisitor; import org.apache.calcite.rel.core.Aggregate; import org.apache.calcite.rel.core.JoinRelType; import org.apache.calcite.rel.core.Sort; import org.apache.calcite.rel.logical.LogicalSort; import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rel.metadata.RelColumnOrigin; +import org.apache.calcite.rel.metadata.RelMetadataQuery; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFamily; import org.apache.calcite.rel.type.RelDataTypeField; @@ -202,6 +207,7 @@ import org.opensearch.sql.expression.function.PPLBuiltinOperators; import org.opensearch.sql.expression.function.PPLFuncImpTable; import org.opensearch.sql.expression.parse.RegexCommonUtils; +import org.opensearch.sql.storage.Table; import org.opensearch.sql.utils.ParseUtils; import org.opensearch.sql.utils.WildcardRenameUtils; @@ -661,18 +667,74 @@ private boolean isMetadataField(String fieldName) { return OpenSearchConstants.METADATAFIELD_TYPE_MAP.containsKey(fieldName); } - /** See logic in {@link org.opensearch.sql.analysis.symbol.SymbolTable#lookupAllFields} */ + /** + * Removes columns that a bare {@code *} should not surface because they are already carried by a + * container column in the same output. + * + *

Mapping semantics apply only to a column that still is a declared field: its name + * is present in {@link Table#getFieldAncestors()} and it reaches this point as a pass-through of + * that table field, established through {@link RelMetadataQuery#getColumnOrigins}. Such a column + * is removed when one of its declared ancestors is also in the row type. The hierarchy comes from + * the mapping rather than from the column name, so it holds for an object mapped with {@code + * disable_objects: true}, where {@code attributes.log.file.path} is a single declared child of + * {@code attributes} with no {@code attributes.log} level in between. See issue 5746. + * + *

A column that merely reuses a declared name is a different value and is kept: an empty or + * derived origin means {@code eval} computed it, so deleting it would silently discard the user's + * value even though the parent struct is in the output. + * + *

Columns whose name no table declares - materialized from a MAP path by {@link + * MapPathPreMaterializer}, renamed by a join, or coming from a table that declares no hierarchy - + * keep the pre-existing convention that a column is nested when the name up to its last dot is + * also a column. So does a pass-through whose declaring table is scanned more than once, where + * table identity cannot say which occurrence a column came from. + * + *

Nothing here is cached across the plan: the hierarchy is read from the tables in the current + * subtree, provenance from the current node, and the presence test from the current row type, so + * none of the three goes stale behind intervening RelNodes. + * + *

See also the equivalent v2 logic in {@link + * org.opensearch.sql.analysis.symbol.SymbolTable#lookupAllFields}, which is name-based only and + * intentionally left unchanged. + */ private static void tryToRemoveNestedFields(CalcitePlanContext context) { - Set allFields = new HashSet<>(context.relBuilder.peek().getRowType().getFieldNames()); - List duplicatedNestedFields = - allFields.stream() - .filter( - field -> { - int lastDot = field.lastIndexOf("."); - return -1 != lastDot && allFields.contains(field.substring(0, lastDot)); - }) - .map(field -> (RexNode) context.relBuilder.field(field)) - .toList(); + RelNode input = context.relBuilder.peek(); + List fieldNames = input.getRowType().getFieldNames(); + Set allFields = new HashSet<>(fieldNames); + ScannedTables scanned = collectScannedTables(input); + RelMetadataQuery mq = input.getCluster().getMetadataQuery(); + + List duplicatedNestedFields = new ArrayList<>(); + for (int i = 0; i < fieldNames.size(); i++) { + String field = fieldNames.get(i); + boolean duplicated = + switch (lineageOf(mq, input, i, field)) { + // Proven to be this table's declared field: the schema decides, unless the table is + // scanned twice and identity cannot say which occurrence this column came from. + case Lineage.PassThrough declared -> + scanned.occurrences(declared.table()) > 1 + ? isNestedByImmediateParent(field, allFields) + : hasAncestorCarryingIt( + mq, input, fieldNames, declared.table(), declared.ancestors(), scanned); + // Proven to be a different value. Sharing a name with a declared field means nothing, + // so never delete it on the schema's behalf. + case Lineage.Computed ignored -> + !scanned.declaresName(field) && isNestedByImmediateParent(field, allFields); + // Not attributable. Use the schema only when the name could not have come from + // anywhere else, otherwise keep the pre-existing convention. + case Lineage.Unknown ignored -> { + Declaration sole = scanned.soleDeclaration(field); + yield sole != null && scanned.occurrences(sole.table()) == 1 + ? hasAncestorCarryingIt( + mq, input, fieldNames, sole.table(), sole.ancestors(), scanned) + : !scanned.declaresName(field) && isNestedByImmediateParent(field, allFields); + } + }; + if (duplicated) { + duplicatedNestedFields.add(context.relBuilder.field(i)); + } + } if (!duplicatedNestedFields.isEmpty()) { // This is a workaround to avoid the bug in Calcite: // In {@link RelBuilder#project_(Iterable, Iterable, Iterable, boolean, Iterable)}, @@ -685,6 +747,193 @@ private static void tryToRemoveNestedFields(CalcitePlanContext context) { } } + /** + * The pre-existing, name-based convention: a column is nested when the name up to its last dot is + * also a column. Retained for every column this pass cannot tie to a declared schema field. + */ + private static boolean isNestedByImmediateParent(String field, Set allFields) { + int lastDot = field.lastIndexOf("."); + return -1 != lastDot && allFields.contains(field.substring(0, lastDot)); + } + + /** + * What is known about where an output column's value comes from. Three states, because "we could + * not tell" must not be confused with "it is a different value". + */ + private sealed interface Lineage { + /** Proven to be {@code table}'s field of the same name, which declares {@code ancestors}. */ + record PassThrough(List table, List ancestors) implements Lineage {} + + /** Proven to be something else: a derived expression, or another field entirely. */ + record Computed() implements Lineage {} + + /** Metadata could not attribute the column to a single source. */ + record Unknown() implements Lineage {} + } + + private static final Lineage COMPUTED = new Lineage.Computed(); + private static final Lineage UNKNOWN = new Lineage.Unknown(); + + /** + * Classifies an output column. {@link Lineage.PassThrough} needs exactly one non-derived origin + * that names this same field on a table declaring it - matching on the name rather than the + * ordinal alone keeps this correct if a scan's row type ever diverges from its table's. + * + *

{@link Lineage.Computed} covers every value the plan builds: no origin at all (a literal), + * any derived origin however many there are (an expression contributes one derived origin per + * column it references), and an origin naming a different field. + * + *

{@link Lineage.Unknown} is only genuine ignorance: metadata declined to answer, as when + * commands such as {@code expand} rebuild a column through a correlate, or several non-derived + * sources feed it, as in a union. + */ + private static Lineage lineageOf( + RelMetadataQuery mq, RelNode input, int ordinal, String fieldName) { + Set origins = mq.getColumnOrigins(input, ordinal); + if (origins == null) { + // Metadata declined to answer. + return UNKNOWN; + } + if (origins.isEmpty()) { + // Answered, and no input column contributed: a literal or other constant. + return COMPUTED; + } + // Derived-ness is decided before cardinality. An expression reports one derived origin per + // column it references, so a computed value legitimately has several origins, and reading that + // as "could not determine" would let the schema delete a value the user just computed. + if (origins.stream().anyMatch(RelColumnOrigin::isDerived)) { + return COMPUTED; + } + if (origins.size() != 1) { + // Several non-derived sources, e.g. a union: no single one of them carries this column. + return UNKNOWN; + } + RelColumnOrigin origin = origins.iterator().next(); + RelOptTable originTable = origin.getOriginTable(); + if (originTable == null) { + return UNKNOWN; + } + List originFieldNames = originTable.getRowType().getFieldNames(); + int originOrdinal = origin.getOriginColumnOrdinal(); + if (originOrdinal < 0 + || originOrdinal >= originFieldNames.size() + || !fieldName.equals(originFieldNames.get(originOrdinal))) { + return COMPUTED; + } + Table table = originTable.unwrap(Table.class); + if (table == null) { + return UNKNOWN; + } + List ancestors = table.getFieldAncestors().get(fieldName); + return ancestors == null + ? COMPUTED + : new Lineage.PassThrough(originTable.getQualifiedName(), ancestors); + } + + /** Whether any of a column's declared ancestors is present in this row as its true container. */ + private static boolean hasAncestorCarryingIt( + RelMetadataQuery mq, + RelNode input, + List fieldNames, + List table, + List ancestors, + ScannedTables scanned) { + return ancestors.stream() + .anyMatch(ancestor -> carriesChild(mq, input, fieldNames, ancestor, table, scanned)); + } + + /** + * Whether the column named {@code ancestorName} in this row is the container carrying the child. + * Proven to be the same table's field of that name, yes; proven computed or proven to be another + * table's, no. When lineage is unknown - {@code expand} and friends rebuild the parent through a + * correlate, leaving nothing to attribute - it is accepted only if that name could not have come + * from anywhere else: declared by the child's table alone, and that table scanned exactly once. A + * name appearing on more than one column is ambiguous and never qualifies. + */ + private static boolean carriesChild( + RelMetadataQuery mq, + RelNode input, + List fieldNames, + String ancestorName, + List childTable, + ScannedTables scanned) { + int ordinal = fieldNames.indexOf(ancestorName); + if (ordinal < 0 || ordinal != fieldNames.lastIndexOf(ancestorName)) { + return false; + } + return switch (lineageOf(mq, input, ordinal, ancestorName)) { + case Lineage.PassThrough ancestor -> ancestor.table().equals(childTable); + case Lineage.Computed ignored -> false; + case Lineage.Unknown ignored -> + scanned.declaredExclusivelyBy(ancestorName, childTable) + && scanned.occurrences(childTable) == 1; + }; + } + + /** One table's declaration of a field name. */ + private record Declaration(List table, List ancestors) {} + + /** + * How each field name is declared by the tables in the given plan, and how many times each table + * is scanned. Walked on demand instead of being recorded when the scan was built, so it reflects + * the plan as it stands. Declarations are kept per table, not unioned, so a name two indices both + * declare is never mistaken for unambiguous. + */ + private record ScannedTables( + Map, List>> declarations, + Multiset> occurrences) { + + boolean declaresName(String name) { + return declarations.containsKey(name); + } + + /** The declaration of this name when exactly one table declares it, else null. */ + @Nullable Declaration soleDeclaration(String name) { + Map, List> byTable = declarations.get(name); + if (byTable == null || byTable.size() != 1) { + return null; + } + Map.Entry, List> only = byTable.entrySet().iterator().next(); + return new Declaration(only.getKey(), only.getValue()); + } + + boolean declaredExclusivelyBy(String name, List table) { + Declaration sole = soleDeclaration(name); + return sole != null && sole.table().equals(table); + } + + int occurrences(List qualifiedTableName) { + return occurrences.count(qualifiedTableName); + } + } + + private static ScannedTables collectScannedTables(RelNode plan) { + Map, List>> declarations = new HashMap<>(); + Multiset> occurrences = HashMultiset.create(); + new RelVisitor() { + @Override + public void visit(RelNode node, int ordinal, RelNode parent) { + RelOptTable relOptTable = node.getTable(); + if (relOptTable != null) { + Table table = relOptTable.unwrap(Table.class); + if (table != null) { + List qualifiedName = relOptTable.getQualifiedName(); + occurrences.add(qualifiedName); + table + .getFieldAncestors() + .forEach( + (name, ancestors) -> + declarations + .computeIfAbsent(name, unused -> new HashMap<>()) + .put(qualifiedName, ancestors)); + } + } + super.visit(node, ordinal, parent); + } + }.go(plan); + return new ScannedTables(declarations, occurrences); + } + /** * Project except with force. * diff --git a/core/src/main/java/org/opensearch/sql/storage/Table.java b/core/src/main/java/org/opensearch/sql/storage/Table.java index 33dbd7d66d3..1ec1682db6e 100644 --- a/core/src/main/java/org/opensearch/sql/storage/Table.java +++ b/core/src/main/java/org/opensearch/sql/storage/Table.java @@ -5,6 +5,7 @@ package org.opensearch.sql.storage; +import java.util.List; import java.util.Map; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.executor.streaming.StreamingSource; @@ -43,6 +44,25 @@ default Map getReservedFieldTypes() { return Map.of(); } + /** + * The container hierarchy declared by the table's schema: one entry for every field name in + * {@link #getFieldTypes()}, mapping it to the names of the container (object/nested) fields that + * declare it, outermost first. A field declared at the root of the schema maps to an empty list. + * + *

This is a structural relationship read from the schema, not a projection list, and it is the + * only authority on which field is nested inside which. It never infers structure from the field + * name, so it stays correct when a schema declares a single field whose name itself contains dots + * — as OpenSearch does for an object mapped with {@code disable_objects: true}, where {@code + * attributes} declares one child literally named {@code log.file.path}, and {@code + * attributes.log.file.path} therefore has the single ancestor {@code attributes}. + * + *

The default is empty, meaning the table declares no hierarchy and callers must fall back to + * their own convention. + */ + default Map> getFieldAncestors() { + return Map.of(); + } + /** * Implement a {@link LogicalPlan} by {@link PhysicalPlan} in storage engine. * 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 c2e1b9330bf..e483646c317 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 @@ -35,6 +35,7 @@ CalciteDateTimeImplementationIT.class, CalciteDedupCommandIT.class, CalciteDescribeCommandIT.class, + CalciteDisableObjectsIT.class, CalciteExpandCommandIT.class, CalciteFieldFormatCommandIT.class, CalciteForeachCommandIT.class, diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisableObjectsIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisableObjectsIT.java new file mode 100644 index 00000000000..c1b7e84058c --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteDisableObjectsIT.java @@ -0,0 +1,331 @@ +/* + * 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 java.io.IOException; +import java.util.Map; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.client.Request; +import org.opensearch.sql.ppl.PPLIntegTestCase; + +/** + * An object mapped with {@code disable_objects: true} declares children whose names keep their dots + * instead of being expanded into intermediate object mappers, so {@code attributes} can declare one + * child literally named {@code log.file.path} and there is no {@code attributes.log} level in + * between. A bare {@code *} must still hide that child behind its parent struct. See issue 5746. + */ +public class CalciteDisableObjectsIT extends PPLIntegTestCase { + + /** disable_objects: true */ + private static final String FLAT = "test-disable-objects"; + + /** control: same document, no disable_objects */ + private static final String NESTED = "test-normal-objects"; + + /** control: same document, disable_objects explicitly false */ + private static final String FALSE = "test-disable-objects-false"; + + /** disable_objects with a scalar child whose name is a prefix of another child */ + private static final String PREFIX = "test-disable-objects-prefix"; + + /** object with dynamic mapping off, so its contents are a MAP the mapping does not declare */ + private static final String UNMAPPED = "test-unmapped-object"; + + /** two scalar roots beside the object, so an expression can reference two distinct columns */ + private static final String TWO_ROOTS = "test-disable-objects-two-roots"; + + /** join probe: declares a scalar {@code attributes}, unrelated to the other index's object */ + private static final String JOIN_LEFT = "test-disable-objects-join-left"; + + /** join probe: declares the object, joined on id */ + private static final String JOIN_RIGHT = "test-disable-objects-join-right"; + + @Override + public void init() throws Exception { + super.init(); + enableCalcite(); + + create( + FLAT, + "{\"mappings\":{\"properties\":{\"attributes\":{\"disable_objects\":true,\"type\":\"object\"}}}}", + "{\"attributes\":{\"log.file.path\":\"/var/log/app.log\",\"logtag\":\"F\"}}"); + create( + NESTED, + "{\"mappings\":{\"properties\":{\"attributes\":{\"type\":\"object\"}}}}", + "{\"attributes\":{\"log.file.path\":\"/var/log/app.log\",\"logtag\":\"F\"}}"); + create( + FALSE, + "{\"mappings\":{\"properties\":{\"attributes\":{\"disable_objects\":false,\"type\":\"object\"}}}}", + "{\"attributes\":{\"log.file.path\":\"/var/log/app.log\",\"logtag\":\"F\"}}"); + create( + PREFIX, + "{\"mappings\":{\"properties\":{\"attributes\":{\"disable_objects\":true,\"type\":\"object\"," + + "\"properties\":{\"log\":{\"type\":\"keyword\"}," + + "\"log.file.path\":{\"type\":\"keyword\"}}}}}}", + "{\"attributes\":{\"log\":\"app\",\"log.file.path\":\"/var/log/app.log\"}}"); + create( + UNMAPPED, + "{\"mappings\":{\"properties\":{\"doc\":{\"type\":\"object\",\"dynamic\":false}}}}", + "{\"doc\":{\"user\":{\"name\":\"alice\"}}}"); + create( + TWO_ROOTS, + "{\"mappings\":{\"properties\":{\"id\":{\"type\":\"keyword\"}," + + "\"tag\":{\"type\":\"keyword\"}," + + "\"attributes\":{\"disable_objects\":true,\"type\":\"object\"}}}}", + "{\"id\":\"a\",\"tag\":\"b\",\"attributes\":{\"log.file.path\":\"/var/log/app.log\"}}"); + create( + JOIN_LEFT, + "{\"mappings\":{\"properties\":{\"id\":{\"type\":\"keyword\"}," + + "\"attributes\":{\"type\":\"keyword\"}}}}", + "{\"id\":\"1\",\"attributes\":\"left-scalar\"}"); + create( + JOIN_RIGHT, + "{\"mappings\":{\"properties\":{\"id\":{\"type\":\"keyword\"}," + + "\"attributes\":{\"disable_objects\":true,\"type\":\"object\"}}}}", + "{\"id\":\"1\",\"attributes\":{\"log.file.path\":\"/var/log/app.log\"}}"); + } + + private void create(String index, String mapping, String document) throws IOException { + Request delete = new Request("DELETE", "/" + index); + delete.addParameter("ignore_unavailable", "true"); + client().performRequest(delete); + + Request create = new Request("PUT", "/" + index); + create.setJsonEntity(mapping); + client().performRequest(create); + + Request doc = new Request("PUT", "/" + index + "/_doc/1?refresh=true"); + doc.setJsonEntity(document); + client().performRequest(doc); + } + + /** Guards the mapping shape the fix relies on: the dotted key stays a single property. */ + @Test + public void disable_objects_keeps_dotted_property_name() throws IOException { + assertTrue( + "expected a single flat property, got " + properties(FLAT), + properties(FLAT).has("log.file.path") && !properties(FLAT).has("log")); + assertTrue( + "expected expanded object properties, got " + properties(NESTED), + properties(NESTED).has("log") && !properties(NESTED).has("log.file.path")); + } + + private JSONObject properties(String index) throws IOException { + return new JSONObject(executeRequest(new Request("GET", "/" + index + "/_mapping"))) + .getJSONObject(index) + .getJSONObject("mappings") + .getJSONObject("properties") + .getJSONObject("attributes") + .getJSONObject("properties"); + } + + @Test + public void no_duplicate_fields_in_schema() throws IOException { + verifySchema(executeQuery("source=" + FLAT), schema("attributes", "struct")); + } + + @Test + public void control_no_disable_objects_has_single_struct_column() throws IOException { + verifySchema(executeQuery("source=" + NESTED), schema("attributes", "struct")); + } + + @Test + public void control_disable_objects_false_has_single_struct_column() throws IOException { + verifySchema(executeQuery("source=" + FALSE), schema("attributes", "struct")); + } + + @Test + public void flat_leaf_is_still_selectable() throws IOException { + JSONObject result = executeQuery("source=" + FLAT + " | fields attributes.log.file.path"); + verifySchema(result, schema("attributes.log.file.path", "string")); + verifyDataRows(result, rows("/var/log/app.log")); + } + + /** + * The parent struct is gone from the row type by the time the implicit trailing {@code *} runs, + * so the group key must survive. Guards against deciding removal from a table-level snapshot + * taken at scan time rather than from the row type in hand. + */ + @Test + public void group_key_survives_when_parent_struct_is_not_in_the_output() throws IOException { + JSONObject result = + executeQuery("source=" + FLAT + " | stats count() by attributes.log.file.path"); + verifySchema(result, schema("count()", "bigint"), schema("attributes.log.file.path", "string")); + verifyDataRows(result, rows(1, "/var/log/app.log")); + } + + @Test + public void sort_keeps_parent_struct_only() throws IOException { + verifySchema( + executeQuery("source=" + FLAT + " | sort attributes.log.file.path"), + schema("attributes", "struct")); + } + + @Test + public void filter_on_flat_leaf_keeps_parent_struct_only() throws IOException { + JSONObject result = + executeQuery("source=" + FLAT + " | where attributes.log.file.path = '/var/log/app.log'"); + verifySchema(result, schema("attributes", "struct")); + verifyDataRows( + result, + rows(Map.of("log", Map.of("file", Map.of("path", "/var/log/app.log")), "logtag", "F"))); + } + + @Test + public void flatten_expands_the_declared_child() throws IOException { + verifyDataRows( + executeQuery("source=" + FLAT + " | flatten attributes | fields `log.file.path`"), + rows("/var/log/app.log")); + } + + /** + * A scalar child whose name is a prefix of another declared child. Both are declared by {@code + * attributes} and so both are carried by it. + */ + @Test + public void scalar_child_sharing_a_prefix_is_also_hidden() throws IOException { + verifySchema(executeQuery("source=" + PREFIX), schema("attributes", "struct")); + } + + /** + * A computed column that reuses a mapped name is not the mapped leaf and must survive, per the PR + * #5351 semantics. The mapped leaf is gone from the row type, so {@code eval} creates a fresh + * column rather than overriding one, and no struct-parent pruning happens on its behalf. + */ + @Test + public void computed_column_reusing_a_mapped_name_survives() throws IOException { + JSONObject result = + executeQuery( + "source=" + FLAT + " | fields attributes | eval `attributes.log.file.path` = 'edited'"); + verifySchema( + result, schema("attributes", "struct"), schema("attributes.log.file.path", "string")); + assertEquals("edited", result.getJSONArray("datarows").getJSONArray(0).getString(1)); + } + + /** + * Same conflation, reached through an exclusion projection, which does not mark a project as + * visited and so still runs the removal pass over the implicit trailing {@code *}. + */ + @Test + public void computed_column_reusing_a_mapped_name_survives_after_exclusion() throws IOException { + JSONObject result = + executeQuery( + "source=" + + FLAT + + " | fields - `attributes.log.file.path` | eval `attributes.log.file.path` =" + + " 'edited'"); + verifySchema( + result, schema("attributes", "struct"), schema("attributes.log.file.path", "string")); + assertEquals("edited", result.getJSONArray("datarows").getJSONArray(0).getString(1)); + } + + /** + * A declared ancestor name must be matched to the scan that actually declares it, not to any + * column that happens to share the name. Here the right index contributes the object's child + * while its own {@code attributes} is projected away inside the subsearch, and the only {@code + * attributes} column in the output is the left index's unrelated keyword. The child belongs to + * the right object, so it must survive. + * + *

End-to-end guard only: join lowering marks a project as visited, so this query does not + * currently reach {@code tryToRemoveNestedFields}. It pins the user-visible contract in case that + * changes; the same-table requirement in {@code carriesChild} is what makes the helper safe. + */ + @Test + public void ancestor_name_from_another_scan_does_not_hide_the_child() throws IOException { + JSONObject result = + executeQuery( + "source=" + + JOIN_LEFT + + " | inner join left=l, right=r ON l.id = r.id [ source=" + + JOIN_RIGHT + + " | fields id, `attributes.log.file.path` ] | fields attributes," + + " `attributes.log.file.path`"); + verifySchema( + result, schema("attributes", "string"), schema("attributes.log.file.path", "string")); + verifyDataRows(result, rows("left-scalar", "/var/log/app.log")); + } + + /** + * The same join without the trailing projection. Also an end-to-end guard rather than a unit of + * this pass, for the same reason. + */ + @Test + public void ancestor_name_from_another_scan_does_not_hide_the_child_without_projection() + throws IOException { + JSONObject result = + executeQuery( + "source=" + + JOIN_LEFT + + " | inner join left=l, right=r ON l.id = r.id [ source=" + + JOIN_RIGHT + + " | fields id, `attributes.log.file.path` ]"); + assertTrue( + "expected the right index's child to survive, got " + result.getJSONArray("schema"), + result.getJSONArray("schema").toString().contains("attributes.log.file.path")); + } + + /** + * Documented limitation. When one plan scans the same index twice, table identity cannot say + * which occurrence a column came from, so pass-throughs of that table fall back to the + * pre-existing immediate-parent rule and the multi-level child stays visible. This keeps the + * scope of the fix to cases the schema can decide unambiguously; it matches the behaviour before + * the fix. + */ + @Test + public void repeated_scan_of_the_same_index_falls_back_to_previous_behaviour() + throws IOException { + verifySchema( + executeQuery("source=" + FLAT + " | append [ source=" + FLAT + " ]"), + schema("attributes", "struct"), + schema("attributes.log.file.path", "string")); + } + + /** + * A computed column reusing a mapped name must survive even when its expression draws on more + * than one input column. Calcite reports one derived origin per referenced column, so such a + * value has several origins - all derived - and must not be mistaken for provenance that could + * not be determined. The parent {@code attributes} stays in the row throughout. + */ + @Test + public void computed_column_from_two_inputs_reusing_a_mapped_name_survives() throws IOException { + JSONObject result = + executeQuery( + "source=" + + TWO_ROOTS + + " | fields - `attributes.log.file.path` | eval `attributes.log.file.path` =" + + " concat(id, tag)"); + verifySchema( + result, + schema("attributes", "struct"), + schema("id", "string"), + schema("tag", "string"), + schema("attributes.log.file.path", "string")); + verifyDataRows( + result, + rows(Map.of("log", Map.of("file", Map.of("path", "/var/log/app.log"))), "a", "b", "ab")); + } + + /** + * A column the mapping does not declare keeps the pre-existing immediate-parent rule, which + * retains it. Here {@code eval} creates a literal {@code doc.user.name} column beside the {@code + * doc} MAP; treating every prefix as a parent would wrongly delete the computed value. + */ + @Test + public void computed_dotted_column_is_retained_beside_its_prefix() throws IOException { + JSONObject result = executeQuery("source=" + UNMAPPED + " | eval `doc.user.name` = 'edited'"); + verifySchema(result, schema("doc", "struct"), schema("doc.user.name", "string")); + // How the unmapped MAP itself renders is incidental; what matters is the computed column + // survived the implicit trailing `*` even though its prefix `doc` is also in the output. + assertEquals("edited", result.getJSONArray("datarows").getJSONArray(0).getString(1)); + } +} diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/DisableObjectsIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/DisableObjectsIT.java new file mode 100644 index 00000000000..7fe2fe5c6b7 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/DisableObjectsIT.java @@ -0,0 +1,69 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl; + +import static org.opensearch.sql.util.MatcherUtils.schema; +import static org.opensearch.sql.util.MatcherUtils.verifySchema; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.client.Request; + +/** + * Pins the v2 engine's behaviour for an object mapped with {@code disable_objects: true}, over both + * the PPL and the SQL endpoint. The duplicate field of issue 5746 is fixed on the + * Calcite engine only (see {@code CalciteDisableObjectsIT}); v2 serves SQL always and PPL when + * Calcite is off, and is deliberately left unchanged. This test exists so that the split is a + * recorded decision rather than an accident. + */ +public class DisableObjectsIT extends PPLIntegTestCase { + + private static final String INDEX = "test_disable_objects_v2"; + + @Override + public void init() throws Exception { + super.init(); // leaves Calcite disabled + + Request delete = new Request("DELETE", "/" + INDEX); + delete.addParameter("ignore_unavailable", "true"); + client().performRequest(delete); + + Request create = new Request("PUT", "/" + INDEX); + create.setJsonEntity( + "{\"mappings\":{\"properties\":{\"attributes\":{\"disable_objects\":true,\"type\":\"object\"}}}}"); + client().performRequest(create); + + Request doc = new Request("PUT", "/" + INDEX + "/_doc/1?refresh=true"); + doc.setJsonEntity("{\"attributes\":{\"log.file.path\":\"/var/log/app.log\",\"logtag\":\"F\"}}"); + client().performRequest(doc); + } + + /** PPL on v2 still surfaces the multi-level child next to its parent struct. */ + @Test + public void ppl_on_v2_still_duplicates_the_multi_level_child() throws IOException { + verifySchema( + executeQuery("source=" + INDEX), + schema("attributes", "struct"), + schema("attributes.log.file.path", "string")); + } + + /** SQL always runs on v2, so it duplicates too. */ + @Test + public void sql_still_duplicates_the_multi_level_child() throws IOException { + JSONObject result = executeJdbcRequest("SELECT * FROM " + INDEX); + verifySchema( + result, schema("attributes", "object"), schema("attributes.log.file.path", "text")); + } + + /** Single-level children are hidden by v2's immediate-parent rule, as before. */ + @Test + public void v2_hides_single_level_children() throws IOException { + assertFalse( + executeQuery("source=" + INDEX).getJSONArray("schema").toString().contains("logtag")); + } +} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java index e3bf799b59b..076edd02fe9 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java @@ -5,10 +5,12 @@ package org.opensearch.sql.opensearch.data.type; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.io.Serializable; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.function.BiConsumer; import lombok.EqualsAndHashCode; @@ -314,6 +316,44 @@ public void accept(Map subtree, String prefix) { return result; } + /** + * Maps every field name produced by {@link #traverseAndFlatten(Map)} to the names of the + * container fields declaring it, outermost first; a field declared at the root of the mapping + * maps to an empty list. Walks the same tree as {@code traverseAndFlatten} but keeps the prefix + * chain instead of discarding it, so a mapping property whose name itself contains dots (an + * object mapped with {@code disable_objects: true}) reports only the containers that actually + * declare it. + * + * @param tree A parsed mapping tree - map between field name and its type. + * @return A map between flattened field name and its ancestor field names. + */ + public static Map> traverseAndCollectAncestors( + Map tree) { + final Map> result = new LinkedHashMap<>(); + BiConsumer, List> visitLevel = + new BiConsumer<>() { + @Override + public void accept(Map subtree, List ancestors) { + for (var entry : subtree.entrySet()) { + String prefix = ancestors.isEmpty() ? "" : ancestors.getLast(); + String nextName = + prefix.isEmpty() + ? entry.getKey() + : String.format("%s.%s", prefix, entry.getKey()); + result.put(nextName, ancestors); + var nextSubtree = entry.getValue().getProperties(); + if (!nextSubtree.isEmpty()) { + accept( + nextSubtree, + ImmutableList.builder().addAll(ancestors).add(nextName).build()); + } + } + } + }; + visitLevel.accept(tree, List.of()); + return result; + } + private static void validateAliasType(Map result) { result.forEach( (key, value) -> { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 8a56fc24e15..8aa073f796b 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -10,6 +10,7 @@ import com.google.common.annotations.VisibleForTesting; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; @@ -89,6 +90,9 @@ public class OpenSearchIndex extends AbstractOpenSearchTable { /** The cached ExprType of fields. */ private Map cachedFieldTypes = null; + /** The cached container hierarchy of fields. */ + private Map> cachedFieldAncestors = null; + /** The cached mapping of alias type field to its original path. */ private Map aliasMapping = null; @@ -157,6 +161,19 @@ public Map getFieldTypes() { return cachedFieldTypes; } + /** + * Derived from the same parsed mapping tree as {@link #getFieldTypes()}, so the two views can + * never disagree about which field is nested inside which. + */ + @Override + public Map> getFieldAncestors() { + if (cachedFieldAncestors == null) { + cachedFieldAncestors = + OpenSearchDataType.traverseAndCollectAncestors(getFieldOpenSearchTypes()); + } + return cachedFieldAncestors; + } + @Override public Map getReservedFieldTypes() { return METADATAFIELD_TYPE_MAP; diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java index 247b7a754e0..c2d2d1b7baa 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java @@ -31,6 +31,7 @@ import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN; import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.MappingType; +import java.util.List; import java.util.Map; import java.util.stream.Stream; import org.apache.commons.lang3.reflect.FieldUtils; @@ -413,6 +414,94 @@ public void text_type_with_fields_ctor() { OpenSearchDataType.of(MappingType.Keyword), type.getFields().get("words"))); } + @Test + public void traverseAndCollectAncestors_covers_every_flattened_field() { + var tree = getSampleMapping(); + var ancestors = OpenSearchDataType.traverseAndCollectAncestors(tree); + + assertAll( + // Same key set as the flattened view, so the two derived views cannot disagree. + () -> + assertEquals(OpenSearchDataType.traverseAndFlatten(tree).keySet(), ancestors.keySet()), + () -> assertEquals(List.of(), ancestors.get("mapping")), + () -> assertEquals(List.of("mapping"), ancestors.get("mapping.keyword")), + () -> assertEquals(List.of("mapping"), ancestors.get("mapping.submapping")), + () -> + assertEquals( + List.of("mapping", "mapping.submapping"), + ancestors.get("mapping.submapping.geo_point")), + () -> + assertEquals( + List.of("mapping", "mapping.submapping", "mapping.submapping.subsubmapping"), + ancestors.get("mapping.submapping.subsubmapping.texttype"))); + } + + /** + * An object mapped with {@code disable_objects: true} declares one child whose name contains + * dots, so the child's only ancestor is the object itself - there is no level in between. See issue 5746. + */ + @Test + public void traverseAndCollectAncestors_multi_level_property_name_has_only_declared_ancestor() { + Map attributes = + Map.of( + "properties", + Map.of( + "log.file.path", Map.of("type", "text"), + "logtag", Map.of("type", "keyword"))); + var tree = Map.of("attributes", OpenSearchDataType.of(MappingType.Object, attributes)); + + var ancestors = OpenSearchDataType.traverseAndCollectAncestors(tree); + + assertAll( + () -> assertEquals(3, ancestors.size()), + () -> assertEquals(List.of(), ancestors.get("attributes")), + () -> assertEquals(List.of("attributes"), ancestors.get("attributes.log.file.path")), + () -> assertEquals(List.of("attributes"), ancestors.get("attributes.logtag")), + // The intermediate levels a name-based rule would assume are absent. + () -> assertFalse(ancestors.containsKey("attributes.log")), + () -> assertFalse(ancestors.containsKey("attributes.log.file"))); + } + + /** A root property whose name contains dots is a root: it has no ancestors. */ + @Test + public void traverseAndCollectAncestors_dotted_root_property_has_no_ancestors() { + var tree = + Map.of( + "s", OpenSearchDataType.of(MappingType.Object), + "s.address", OpenSearchDataType.of(MappingType.Keyword)); + + var ancestors = OpenSearchDataType.traverseAndCollectAncestors(tree); + + assertAll( + () -> assertEquals(List.of(), ancestors.get("s")), + () -> assertEquals(List.of(), ancestors.get("s.address"))); + } + + /** Text multi-fields live under "fields", not "properties", so they are not part of the tree. */ + @Test + public void traverseAndCollectAncestors_excludes_text_multi_fields() { + var ancestors = + OpenSearchDataType.traverseAndCollectAncestors(Map.of("source", textKeywordType)); + + assertAll( + () -> assertEquals(Map.of("source", List.of()), ancestors), + () -> assertFalse(ancestors.containsKey("source.keyword"))); + } + + @Test + public void traverseAndCollectAncestors_covers_nested_type_children() { + Map projects = + Map.of("type", "nested", "properties", Map.of("name", Map.of("type", "keyword"))); + var tree = Map.of("projects", OpenSearchDataType.of(MappingType.Nested, projects)); + + var ancestors = OpenSearchDataType.traverseAndCollectAncestors(tree); + + assertAll( + () -> assertEquals(List.of(), ancestors.get("projects")), + () -> assertEquals(List.of("projects"), ancestors.get("projects.name"))); + } + private Map getSampleMapping() { Map subsubmapping = Map.of(