From c8f2103c6909647fbb1833c27775f38aae233355 Mon Sep 17 00:00:00 2001 From: Isha Gupta Date: Mon, 27 Apr 2026 15:59:51 +0800 Subject: [PATCH] Add include_metadata request parameter for PPL queries #5235 Signed-off-by: Isha Gupta --- .../api/parser/UnifiedQueryParserTest.java | 10 +- .../opensearch/sql/ast/statement/Query.java | 1 + .../sql/calcite/CalcitePlanContext.java | 15 +++ .../sql/calcite/CalciteRelNodeVisitor.java | 126 +++++++++++++++--- .../opensearch/sql/executor/QueryService.java | 60 ++++++++- .../sql/executor/execution/QueryPlan.java | 36 ++++- .../executor/execution/QueryPlanFactory.java | 6 +- .../execution/QueryPlanFactoryTest.java | 8 +- .../sql/executor/execution/QueryPlanTest.java | 8 +- .../calcite/remote/CalciteEvalCommandIT.java | 102 ++++++++++++++ .../remote/CalcitePPLSpathCommandIT.java | 23 ++++ .../rest-api-spec/test/issues/5185.yml | 69 ++++++++++ .../request/PPLQueryRequestFactory.java | 8 ++ .../sql/plugin/rest/RestPPLQueryAction.java | 3 +- .../org/opensearch/sql/ppl/PPLService.java | 4 + .../sql/ppl/domain/PPLQueryRequest.java | 15 +++ .../sql/ppl/parser/AstStatementBuilder.java | 13 +- .../sql/ppl/utils/UnresolvedPlanHelper.java | 4 +- .../opensearch/sql/ppl/PPLServiceTest.java | 36 ----- .../ppl/calcite/CalcitePPLFlattenTest.java | 28 ++-- .../sql/ppl/calcite/CalcitePPLSpathTest.java | 28 ++++ .../ppl/parser/AstStatementBuilderTest.java | 42 +++--- .../ppl/utils/UnresolvedPlanHelperTest.java | 4 +- .../sql/sql/parser/AstStatementBuilder.java | 2 +- 24 files changed, 539 insertions(+), 112 deletions(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5185.yml diff --git a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java index 1b6b5181aef..40b3aa948fa 100644 --- a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java +++ b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java @@ -11,7 +11,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.agg; import static org.opensearch.sql.ast.dsl.AstDSL.aggregate; import static org.opensearch.sql.ast.dsl.AstDSL.alias; -import static org.opensearch.sql.ast.dsl.AstDSL.allFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import static org.opensearch.sql.ast.dsl.AstDSL.compare; import static org.opensearch.sql.ast.dsl.AstDSL.defaultStatsArgs; import static org.opensearch.sql.ast.dsl.AstDSL.eval; @@ -36,7 +36,7 @@ public class UnifiedQueryParserTest extends UnifiedQueryTestBase { public void testParseSource() { assertEqual( "source = catalog.employees", - project(relation(qualifiedName("catalog", "employees")), allFields())); + project(relation(qualifiedName("catalog", "employees")), AllFieldsExcludeMeta.of())); } @Test @@ -47,7 +47,7 @@ public void testParseFilter() { filter( relation(qualifiedName("catalog", "employees")), compare(">", field("age"), intLiteral(30))), - allFields())); + AllFieldsExcludeMeta.of())); } @Test @@ -58,7 +58,7 @@ public void testParseEval() { eval( relation(qualifiedName("catalog", "employees")), let(field("f"), function("abs", field("id")))), - allFields())); + AllFieldsExcludeMeta.of())); } @Test @@ -72,7 +72,7 @@ public void testParseStats() { emptyList(), exprList(alias("department", field("department"))), defaultStatsArgs()), - allFields())); + AllFieldsExcludeMeta.of())); } @Test diff --git a/core/src/main/java/org/opensearch/sql/ast/statement/Query.java b/core/src/main/java/org/opensearch/sql/ast/statement/Query.java index c6a78724b76..9f03e65d290 100644 --- a/core/src/main/java/org/opensearch/sql/ast/statement/Query.java +++ b/core/src/main/java/org/opensearch/sql/ast/statement/Query.java @@ -26,6 +26,7 @@ public class Query extends Statement { protected final UnresolvedPlan plan; protected final int fetchSize; private final QueryType queryType; + private final boolean includeMetadata; private HighlightConfig highlightConfig; @Override diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java b/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java index 5f58dea227e..f71532bc32d 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java @@ -59,6 +59,13 @@ public class CalcitePlanContext { */ @Getter @Setter private boolean isProjectVisited = false; + /** + * Whether to include metadata fields like _id, _index, _score in the result. When true, metadata + * fields are included in wildcard field selections. When false (default), metadata fields are + * excluded. + */ + @Getter @Setter private boolean includeMetadata = false; + private final Stack correlVar = new Stack<>(); private final Stack> windowPartitions = new Stack<>(); @@ -99,6 +106,7 @@ private CalcitePlanContext(CalcitePlanContext parent) { this.rexBuilder = parent.rexBuilder; // Share the same rexBuilder this.functionProperties = parent.functionProperties; this.highlightConfig = parent.highlightConfig; + this.includeMetadata = parent.includeMetadata; // Preserve parent's metadata setting this.rexLambdaRefMap = new HashMap<>(); // New map for lambda variables this.capturedVariables = new ArrayList<>(); // New list for captured variables this.inLambdaContext = true; // Mark that we're inside a lambda @@ -147,6 +155,13 @@ public static CalcitePlanContext create( return new CalcitePlanContext(config, sysLimit, queryType); } + public static CalcitePlanContext create( + FrameworkConfig config, SysLimit sysLimit, QueryType queryType, boolean includeMetadata) { + CalcitePlanContext context = new CalcitePlanContext(config, sysLimit, queryType); + context.setIncludeMetadata(includeMetadata); + return context; + } + /** * Executes {@code action} with the thread-local legacy flag set according to the supplied * settings. 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 30f6d53eff0..5b33a05b57c 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -493,11 +493,18 @@ private RelNode handleAllFieldsProject(Project node, CalcitePlanContext context) "Invalid field exclusion: operation would exclude all fields from the result set"); } AllFields allFields = (AllFields) node.getProjectList().getFirst(); - if (!(allFields instanceof AllFieldsExcludeMeta)) { - // Should not remove nested fields for AllFieldsExcludeMeta. + + if (allFields instanceof AllFieldsExcludeMeta) { + // Do NOT remove nested fields for AllFieldsExcludeMeta + tryToRemoveMetaFields(context, true); // Force exclude metadata fields + } else { + // For AllFields (include_metadata=true), include metadata fields tryToRemoveNestedFields(context); + // Mark as project visited to prevent automatic metadata field removal + context.setProjectVisited(true); + // Don't force exclude metadata fields - let them remain + tryToRemoveMetaFields(context, false); } - tryToRemoveMetaFields(context, allFields instanceof AllFieldsExcludeMeta); return context.relBuilder.peek(); } @@ -645,6 +652,11 @@ private static void forceProjectExcept(RelBuilder relBuilder, Iterable * @param excludeByForce whether exclude metadata fields by force */ private static void tryToRemoveMetaFields(CalcitePlanContext context, boolean excludeByForce) { + // If include_metadata=true, never remove metadata fields + if (context.isIncludeMetadata() && !excludeByForce) { + return; + } + if (excludeByForce || !context.isProjectVisited()) { List originalFields = context.relBuilder.peek().getRowType().getFieldNames(); List metaFieldsRef = @@ -951,10 +963,60 @@ public RelNode visitBin(Bin node, CalcitePlanContext context) { String alias = node.getAlias() != null ? node.getAlias() : fieldName; projectPlusOverriding(List.of(binExpression), List.of(alias), context); + dropStructParentsFor(alias, context); return context.relBuilder.peek(); } + /** + * If {@code dottedName} addresses a nested leaf inside a struct that OpenSearch has exposed + * through both its struct-parent columns and its flattened leaf columns (e.g. the telemetry + * mapping exposes {@code resource}, {@code resource.attributes}, ..., {@code + * resource.attributes.telemetry.sdk.version} side-by-side), drop the struct-parent prefixes from + * the current row. This keeps a subsequent {@link #tryToRemoveNestedFields(CalcitePlanContext)} + * pass from collapsing the flattened leaves back into the parents when the final implicit {@code + * fields *} projection runs. + * + *

This preserves the behaviour that issue #4482 originally required for {@code bin} on a + * nested field without an explicit {@code fields} projection. It is invoked from two places: + * + *

    + *
  • {@link #projectPlusOverriding(List, List, CalcitePlanContext)} — for every override whose + * new name exactly matched a pre-existing column. This catches {@code eval} (and every + * other command that funnels through {@code projectPlusOverriding}) assigning to an + * existing flattened nested leaf. + *
  • {@link #visitBin(Bin, CalcitePlanContext)} — defensively, so that {@code bin} keeps + * dropping struct parents even when the alias happens not to match an existing field name + * (e.g. when the user supplied a custom alias). This is also what the regression test in + * {@code CalciteBinCommandIT#testBinWithNestedFieldWithoutExplicitProjection} exercises. + *
+ * + * Using this narrowly-scoped pruning instead of a global prefix-override in {@link + * #shouldOverrideField} is what keeps issue #5185 and the reviewer's {@code eval agent.name = + * ...} case safe. + * + *

No-op when no such struct-parent columns exist (e.g. flat columns or MAP roots from {@code + * spath}). + */ + private void dropStructParentsFor(String dottedName, CalcitePlanContext context) { + if (dottedName == null || dottedName.indexOf('.') < 0) { + return; + } + List fieldNames = context.relBuilder.peek().getRowType().getFieldNames(); + List parentsToDrop = new ArrayList<>(); + int dotIdx = dottedName.indexOf('.'); + while (dotIdx >= 0) { + String prefix = dottedName.substring(0, dotIdx); + if (fieldNames.contains(prefix)) { + parentsToDrop.add(context.relBuilder.field(prefix)); + } + dotIdx = dottedName.indexOf('.', dotIdx + 1); + } + if (!parentsToDrop.isEmpty()) { + context.relBuilder.projectExcept(parentsToDrop); + } + } + @Override public RelNode visitParse(Parse node, CalcitePlanContext context) { visitChildren(node, context); @@ -1270,12 +1332,12 @@ private RelNode buildConversionProjection(ConversionState state, CalcitePlanCont private void projectPlusOverriding( List newFields, List newNames, CalcitePlanContext context) { - List originalFieldNames = context.relBuilder.peek().getRowType().getFieldNames(); + Set originalFieldNameSet = + new HashSet<>(context.relBuilder.peek().getRowType().getFieldNames()); + List overriddenNames = + newNames.stream().filter(originalFieldNameSet::contains).toList(); List toOverrideList = - originalFieldNames.stream() - .filter(originalName -> shouldOverrideField(originalName, newNames)) - .map(a -> (RexNode) context.relBuilder.field(a)) - .toList(); + overriddenNames.stream().map(a -> (RexNode) context.relBuilder.field(a)).toList(); // 1. add the new fields, For example "age0, country0" context.relBuilder.projectPlus(newFields); // 2. drop the overriding field list, it's duplicated now. For example "age, country" @@ -1291,17 +1353,49 @@ private void projectPlusOverriding( expectedRenameFields.addAll(newNames); // 5. rename context.relBuilder.rename(expectedRenameFields); + // 6. For each overridden dotted-path name that matched an existing flattened nested leaf, + // prune the struct-parent columns that OpenSearch exposed side-by-side with that leaf. Without + // this, a downstream implicit `fields *` invokes `tryToRemoveNestedFields`, which would drop + // the freshly-assigned dotted leaf back out again because its struct-parent prefix is still in + // the row schema (see issue #4482 and the scratch coverage in CalciteEvalCommandIT). + // + // Gating on "the override actually fired" is what keeps the reviewer's PR #5351 case safe: + // `source=idx | fields agent | eval agent.name = "test"` has no pre-existing `agent.name` + // column, so overriddenNames is empty and the struct-parent `agent` survives untouched. + // It also keeps issue #5185 safe — spath introduces a MAP root and subsequent eval assigns + // to brand-new dotted paths that were not already in the row schema. + for (String overridden : overriddenNames) { + dropStructParentsFor(overridden, context); + } } + /** + * Determine whether the column {@code originalName} should be replaced when a batch of new + * columns named {@code newNames} is being added. Only exact-name matches count as overrides — + * {@code eval foo.bar = ...} creates a brand new field literally named {@code foo.bar} and must + * never drop sibling or parent fields. This mirrors SPL1 semantics, where assigning a dotted name + * introduces a literal column of that name without touching any other field. + * + *

Earlier revisions (see PR #4606 / #5351) attempted to broaden this to a {@code + * newName.startsWith(originalName + ".")} prefix match. That prefix branch silently dropped any + * column that happened to be a prefix of an eval target, which caused two regressions: + * + *

    + *
  • Issue #5185 — a MAP-typed root column produced by {@code spath} got dropped when eval + * introduced multiple dotted-path fields under it. + *
  • The reviewer's case on PR #5351 — {@code source=big5 | fields agent | eval agent.name = + * "test"} dropped the {@code agent} column entirely. + *
+ * + * Struct-parent pruning for the "override on a real flattened nested leaf" case is handled + * uniformly in {@link #projectPlusOverriding(List, List, CalcitePlanContext)}, which invokes + * {@link #dropStructParentsFor(String, CalcitePlanContext)} only for overrides that actually + * replaced an existing column. This keeps issue #4482 fixed across every command that funnels + * through {@code projectPlusOverriding} (bin, eval, rex/sed, trendline, expand, flatten, + * patterns) without reintroducing the #5185 / reviewer regressions here. + */ private boolean shouldOverrideField(String originalName, List newNames) { - return newNames.stream() - .anyMatch( - newName -> - // Match exact field names (e.g., "age" == "age") for flat fields - newName.equals(originalName) - // OR match nested paths (e.g., "resource.attributes..." starts with - // "resource.") - || newName.startsWith(originalName + ".")); + return newNames.contains(originalName); } private List> extractInputRefList(List aggCalls) { diff --git a/core/src/main/java/org/opensearch/sql/executor/QueryService.java b/core/src/main/java/org/opensearch/sql/executor/QueryService.java index b294a168cd8..9d4a75e8129 100644 --- a/core/src/main/java/org/opensearch/sql/executor/QueryService.java +++ b/core/src/main/java/org/opensearch/sql/executor/QueryService.java @@ -100,9 +100,22 @@ public void execute( QueryType queryType, HighlightConfig highlightConfig, ResponseListener listener) { + execute(plan, queryType, highlightConfig, false, listener); + } + + /** Execute with optional highlight config and include metadata flag. */ + public void execute( + UnresolvedPlan plan, + QueryType queryType, + HighlightConfig highlightConfig, + boolean includeMetadata, + ResponseListener listener) { if (shouldUseCalcite(queryType)) { - executeWithCalcite(plan, queryType, highlightConfig, listener); + executeWithCalcite(plan, queryType, highlightConfig, includeMetadata, listener); } else { + // Legacy engine always includes basic metadata (schema information) + // The includeMetadata flag doesn't affect legacy engine behavior since + // it already provides column names, types, and aliases in the schema executeWithLegacy(plan, queryType, listener, Optional.empty()); } } @@ -123,9 +136,22 @@ public void explain( HighlightConfig highlightConfig, ResponseListener listener, ExplainMode mode) { + explain(plan, queryType, highlightConfig, false, listener, mode); + } + + /** Explain with optional highlight config and include metadata flag. */ + public void explain( + UnresolvedPlan plan, + QueryType queryType, + HighlightConfig highlightConfig, + boolean includeMetadata, + ResponseListener listener, + ExplainMode mode) { if (shouldUseCalcite(queryType)) { - explainWithCalcite(plan, queryType, highlightConfig, listener, mode); + explainWithCalcite(plan, queryType, highlightConfig, includeMetadata, listener, mode); } else { + // Legacy engine provides basic explain information + // The includeMetadata flag doesn't significantly affect legacy explain behavior explainWithLegacy(plan, queryType, listener, mode, Optional.empty()); } } @@ -135,6 +161,15 @@ public void executeWithCalcite( QueryType queryType, HighlightConfig highlightConfig, ResponseListener listener) { + executeWithCalcite(plan, queryType, highlightConfig, false, listener); + } + + public void executeWithCalcite( + UnresolvedPlan plan, + QueryType queryType, + HighlightConfig highlightConfig, + boolean includeMetadata, + ResponseListener listener) { CalcitePlanContext.run( () -> { try { @@ -144,7 +179,10 @@ public void executeWithCalcite( long analyzeStart = System.nanoTime(); CalcitePlanContext context = CalcitePlanContext.create( - buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); + buildFrameworkConfig(), + SysLimit.fromSettings(settings), + queryType, + includeMetadata); context.setHighlightConfig(highlightConfig); @@ -172,6 +210,7 @@ public void executeWithCalcite( } catch (Throwable t) { if (isCalciteFallbackAllowed(t) && !(t instanceof NonFallbackCalciteException)) { log.warn("Fallback to V2 query engine since got exception", t); + // Legacy engine provides basic metadata support, so fallback is acceptable executeWithLegacy(plan, queryType, listener, Optional.of(t)); } else { propagateCalciteError(t, listener); @@ -187,13 +226,26 @@ public void explainWithCalcite( HighlightConfig highlightConfig, ResponseListener listener, ExplainMode mode) { + explainWithCalcite(plan, queryType, highlightConfig, false, listener, mode); + } + + public void explainWithCalcite( + UnresolvedPlan plan, + QueryType queryType, + HighlightConfig highlightConfig, + boolean includeMetadata, + ResponseListener listener, + ExplainMode mode) { CalcitePlanContext.run( () -> { try { QueryProfiling.noop(); CalcitePlanContext context = CalcitePlanContext.create( - buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); + buildFrameworkConfig(), + SysLimit.fromSettings(settings), + queryType, + includeMetadata); context.setHighlightConfig(highlightConfig); context.run( () -> { diff --git a/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java b/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java index 3f6407e8873..f9be6635a7c 100644 --- a/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java +++ b/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java @@ -32,6 +32,8 @@ public class QueryPlan extends AbstractPlan { protected final HighlightConfig highlightConfig; + protected final boolean includeMetadata; + /** Constructor. */ public QueryPlan( QueryId queryId, @@ -39,7 +41,7 @@ public QueryPlan( UnresolvedPlan plan, QueryService queryService, ResponseListener listener) { - this(queryId, queryType, plan, queryService, listener, null); + this(queryId, queryType, plan, queryService, listener, null, false); } /** Constructor with highlight config. */ @@ -50,12 +52,25 @@ public QueryPlan( QueryService queryService, ResponseListener listener, HighlightConfig highlightConfig) { + this(queryId, queryType, plan, queryService, listener, highlightConfig, false); + } + + /** Constructor with highlight config and include metadata flag. */ + public QueryPlan( + QueryId queryId, + QueryType queryType, + UnresolvedPlan plan, + QueryService queryService, + ResponseListener listener, + HighlightConfig highlightConfig, + boolean includeMetadata) { super(queryId, queryType); this.plan = plan; this.queryService = queryService; this.listener = listener; this.pageSize = Optional.empty(); this.highlightConfig = highlightConfig; + this.includeMetadata = includeMetadata; } /** Constructor with page size. */ @@ -66,20 +81,33 @@ public QueryPlan( int pageSize, QueryService queryService, ResponseListener listener) { + this(queryId, queryType, plan, pageSize, queryService, listener, false); + } + + /** Constructor with page size and include metadata flag. */ + public QueryPlan( + QueryId queryId, + QueryType queryType, + UnresolvedPlan plan, + int pageSize, + QueryService queryService, + ResponseListener listener, + boolean includeMetadata) { super(queryId, queryType); this.plan = plan; this.queryService = queryService; this.listener = listener; this.pageSize = Optional.of(pageSize); this.highlightConfig = null; + this.includeMetadata = includeMetadata; } @Override public void execute() { if (pageSize.isPresent()) { - queryService.execute(new Paginate(pageSize.get(), plan), getQueryType(), listener); + queryService.execute(new Paginate(pageSize.get(), plan), getQueryType(), highlightConfig, includeMetadata, listener); } else { - queryService.execute(plan, getQueryType(), highlightConfig, listener); + queryService.execute(plan, getQueryType(), highlightConfig, includeMetadata, listener); } } @@ -91,7 +119,7 @@ public void explain( new NotImplementedException( "`explain` feature for paginated requests is not implemented yet.")); } else { - queryService.explain(plan, getQueryType(), highlightConfig, listener, mode); + queryService.explain(plan, getQueryType(), highlightConfig, includeMetadata, listener, mode); } } } diff --git a/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java b/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java index 48e2b3ce5e0..c55f7dce39a 100644 --- a/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java +++ b/core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java @@ -115,7 +115,8 @@ public AbstractPlan visitQuery( node.getPlan(), node.getFetchSize(), queryService, - context.getLeft()); + context.getLeft(), + node.isIncludeMetadata()); } else { // This should be picked up by the legacy engine. throw new UnsupportedCursorRequestException(); @@ -127,7 +128,8 @@ public AbstractPlan visitQuery( node.getPlan(), queryService, context.getLeft(), - node.getHighlightConfig()); + node.getHighlightConfig(), + node.isIncludeMetadata()); } } diff --git a/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanFactoryTest.java b/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanFactoryTest.java index dd73b26a8c3..ecbc03429d5 100644 --- a/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanFactoryTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanFactoryTest.java @@ -59,14 +59,14 @@ void init() { @Test public void create_from_query_should_success() { - Statement query = new Query(plan, 0, queryType); + Statement query = new Query(plan, 0, queryType, false); AbstractPlan queryExecution = factory.create(query, queryListener, explainListener); assertTrue(queryExecution instanceof QueryPlan); } @Test public void create_from_explain_should_success() { - Statement query = new Explain(new Query(plan, 0, queryType), queryType); + Statement query = new Explain(new Query(plan, 0, queryType, false), queryType); AbstractPlan queryExecution = factory.create(query, queryListener, explainListener); assertTrue(queryExecution instanceof ExplainPlan); } @@ -103,7 +103,7 @@ public void no_consumer_response_channel() { public void create_query_with_fetch_size_which_can_be_paged() { when(plan.accept(any(CanPaginateVisitor.class), any())).thenReturn(Boolean.TRUE); factory = new QueryPlanFactory(queryService); - Statement query = new Query(plan, 10, queryType); + Statement query = new Query(plan, 10, queryType, false); AbstractPlan queryExecution = factory.create(query, queryListener, explainListener); assertTrue(queryExecution instanceof QueryPlan); } @@ -112,7 +112,7 @@ public void create_query_with_fetch_size_which_can_be_paged() { public void create_query_with_fetch_size_which_cannot_be_paged() { when(plan.accept(any(CanPaginateVisitor.class), any())).thenReturn(Boolean.FALSE); factory = new QueryPlanFactory(queryService); - Statement query = new Query(plan, 10, queryType); + Statement query = new Query(plan, 10, queryType, false); assertThrows( UnsupportedCursorRequestException.class, () -> factory.create(query, queryListener, explainListener)); diff --git a/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java b/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java index 128df14ff8e..b6e14a63fd4 100644 --- a/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java @@ -9,6 +9,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -52,7 +55,7 @@ public void execute_no_page_size() { QueryPlan query = new QueryPlan(queryId, queryType, plan, queryService, queryListener); query.execute(); - verify(queryService, times(1)).execute(any(), any(), any(), any()); + verify(queryService, times(1)).execute(any(), any(), any(), anyBoolean(), any()); } @Test @@ -60,7 +63,8 @@ public void explain_no_page_size() { QueryPlan query = new QueryPlan(queryId, queryType, plan, queryService, queryListener); query.explain(explainListener, mode); - verify(queryService, times(1)).explain(plan, queryType, null, explainListener, mode); + verify(queryService, times(1)) + .explain(eq(plan), eq(queryType), isNull(), anyBoolean(), eq(explainListener), eq(mode)); } @Test diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java index 588a4a784f9..219020b1650 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteEvalCommandIT.java @@ -6,11 +6,13 @@ package org.opensearch.sql.calcite.remote; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_TELEMETRY; 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 com.google.common.collect.ImmutableMap; import java.io.IOException; import org.json.JSONObject; import org.junit.jupiter.api.Test; @@ -25,6 +27,7 @@ public void init() throws Exception { enableCalcite(); loadIndex(Index.BANK); + loadIndex(Index.TELEMETRY); // Create test data for string concatenation Request request1 = new Request("PUT", "/test_eval/_doc/1?refresh=true"); @@ -38,6 +41,21 @@ public void init() throws Exception { Request request3 = new Request("PUT", "/test_eval/_doc/3?refresh=true"); request3.setJsonEntity("{\"name\": \"Charlie\", \"age\": null, \"title\": \"Analyst\"}"); client().performRequest(request3); + + // Index with a struct field `agent` to reproduce the reviewer's case from PR #5351: + // source= | fields agent | eval agent.name = "test" + // Rely on dynamic mapping — OpenSearch infers `agent` as an object with string children + // from the document contents. Using dynamic mapping keeps the init idempotent across + // repeated `@Before` invocations in the preserved cluster. + Request agentDoc1 = new Request("PUT", "/test_eval_agent/_doc/1?refresh=true"); + agentDoc1.setJsonEntity( + "{\"agent\": {\"name\": \"winlogbeat\", \"version\": \"7.0\"}, \"message\": \"hello\"}"); + client().performRequest(agentDoc1); + + Request agentDoc2 = new Request("PUT", "/test_eval_agent/_doc/2?refresh=true"); + agentDoc2.setJsonEntity( + "{\"agent\": {\"name\": \"filebeat\", \"version\": \"8.1\"}, \"message\": \"world\"}"); + client().performRequest(agentDoc2); } @Test @@ -86,6 +104,90 @@ public void testEvalStringConcatenationWithLiterals() throws IOException { rows("Charlie", "Analyst", "Name: Charlie, Title: Analyst")); } + @Test + public void testEvalDottedNameDoesNotDropStructParent() throws IOException { + // Reviewer's case from PR #5351: assigning a new dotted-path column must not remove the + // struct-parent column that happens to be a prefix of the eval target. + // Equivalent SPL1 query: + // source= | fields agent | eval agent.name = "test" + // Before the fix, the prefix-override in shouldOverrideField silently dropped the `agent` + // column entirely from the result schema. With the fix, `agent` is preserved. + // The newly-created literal column `agent.name` is also available (verified via an + // explicit trailing `fields` projection that bypasses tryToRemoveNestedFields). + JSONObject result = + executeQuery( + "source=test_eval_agent | fields agent | eval `agent.name` = 'test' | fields agent," + + " `agent.name`"); + verifySchema(result, schema("agent", "struct"), schema("agent.name", "string")); + verifyDataRows( + result, + rows(ImmutableMap.of("name", "winlogbeat", "version", "7.0"), "test"), + rows(ImmutableMap.of("name", "filebeat", "version", "8.1"), "test")); + } + + @Test + public void testEvalDottedNamePreservesStructParent_ImplicitProject() throws IOException { + // Complementary coverage for the reviewer's case without the explicit trailing projection. + // With the implicit `fields *` (AllFields) that the PPL parser appends, the downstream + // `tryToRemoveNestedFields` pass still collapses the flattened leaf back into its struct + // parent -- but the important regression guard is that the struct parent `agent` is no + // longer dropped by `shouldOverrideField`'s prefix branch. + JSONObject result = + executeQuery("source=test_eval_agent | fields agent | eval `agent.name` = 'test'"); + verifySchema(result, schema("agent", "struct")); + } + + @Test + public void testEvalOverrideOfFlattenedNestedLeafSurvivesImplicitProject() throws IOException { + // Regression guard for PR #5351: eval assigning a new value to a dotted name that matches an + // existing OpenSearch flattened nested leaf must not have that value silently eaten by the + // implicit `fields *` pass. + // + // The telemetry mapping exposes struct parents (resource, resource.attributes, ..., + // resource.attributes.telemetry.sdk) side-by-side with the flattened leaves. When eval + // overrides the leaf, projectPlusOverriding now prunes those struct parents so a subsequent + // tryToRemoveNestedFields pass does not delete the overridden leaf on the way out. + // + // Before the fix, this query returned rows with the original `resource` struct (still + // containing the pre-override integer version) and no `resource.attributes.telemetry.sdk.*` + // flattened leaves at all -- the "OVERRIDE" string was completely lost. + JSONObject result = + executeQuery( + String.format( + "source=%s | eval `resource.attributes.telemetry.sdk.version` = 'OVERRIDE'", + TEST_INDEX_TELEMETRY)); + + verifyDataRows( + result, + rows(true, "java", "opentelemetry", 9, "OVERRIDE"), + rows(false, "python", "opentelemetry", 12, "OVERRIDE"), + rows(true, "javascript", "opentelemetry", 9, "OVERRIDE"), + rows(false, "go", "opentelemetry", 16, "OVERRIDE"), + rows(true, "rust", "opentelemetry", 12, "OVERRIDE")); + } + + @Test + public void testEvalOverrideOfFlattenedNestedLeafWithExplicitProject() throws IOException { + // Control for the test above: with an explicit trailing `fields` projection, the implicit + // `fields *` codepath (and tryToRemoveNestedFields) does not run, so eval always returned + // the overridden value even before the fix. This test pins the user-facing contract for the + // explicit-projection variant regardless of internal pruning behaviour. + JSONObject result = + executeQuery( + String.format( + "source=%s | eval `resource.attributes.telemetry.sdk.version` = 'OVERRIDE' | fields" + + " `resource.attributes.telemetry.sdk.version`", + TEST_INDEX_TELEMETRY)); + verifySchema(result, schema("resource.attributes.telemetry.sdk.version", "string")); + verifyDataRows( + result, + rows("OVERRIDE"), + rows("OVERRIDE"), + rows("OVERRIDE"), + rows("OVERRIDE"), + rows("OVERRIDE")); + } + @Test public void testEvalStringConcatenationWithExistingData() throws IOException { JSONObject result = diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java index ec6f8583b23..0bd7ac803f9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLSpathCommandIT.java @@ -216,4 +216,27 @@ public void testSpathAutoExtractWithSort() throws IOException { verifySchema(result, schema("doc.user.name", "string")); verifyDataRowsInOrder(result, rows("Alice"), rows("John")); } + + @Test + public void testSpathAutoExtractWithMultiFieldEval() throws IOException { + JSONObject result = + executeQuery( + "source=test_spath_cmd | spath input=doc" + + " | eval doc.user.name=doc.user.name, doc.user.age=doc.user.age" + + " | fields doc.user.name, doc.user.age"); + verifySchema(result, schema("doc.user.name", "string"), schema("doc.user.age", "string")); + verifyDataRows(result, rows("Alice", "25"), rows("John", "30")); + } + + @Test + public void testSpathAutoExtractWithSeparateEvalCommands() throws IOException { + JSONObject result = + executeQuery( + "source=test_spath_cmd | spath input=doc" + + " | eval doc.user.name=doc.user.name" + + " | eval doc.user.age=doc.user.age" + + " | fields doc.user.name, doc.user.age"); + verifySchema(result, schema("doc.user.name", "string"), schema("doc.user.age", "string")); + verifyDataRows(result, rows("Alice", "25"), rows("John", "30")); + } } diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5185.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5185.yml new file mode 100644 index 00000000000..0f939a03585 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5185.yml @@ -0,0 +1,69 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: true + - do: + indices.create: + index: issue5185 + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + doc: + type: text + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "issue5185", "_id": "1"}}' + - '{"doc": "{\"user\":{\"name\":\"John\",\"age\":30}}"}' + - '{"index": {"_index": "issue5185", "_id": "2"}}' + - '{"doc": "{\"user\":{\"name\":\"Alice\",\"age\":25}}"}' + +--- +teardown: + - do: + indices.delete: + index: issue5185 + ignore_unavailable: true + - do: + query.settings: + body: + transient: + plugins.calcite.enabled: false + +--- +"Issue 5185: eval with multiple dotted-path assignments from MAP column": + - skip: + features: + - headers + - allowed_warnings + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=issue5185 | spath input=doc | eval doc.user.name=doc.user.name, doc.user.age=doc.user.age | fields doc.user.name, doc.user.age" + + - match: { total: 2 } + - length: { datarows: 2 } + +--- +"Issue 5185: separate eval commands with dotted-path from MAP column": + - skip: + features: + - headers + - allowed_warnings + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: "source=issue5185 | spath input=doc | eval doc.user.name=doc.user.name | eval doc.user.age=doc.user.age | fields doc.user.name, doc.user.age" + + - match: { total: 2 } + - length: { datarows: 2 } diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java b/plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java index bb87bf7fa91..e158ecb3937 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java @@ -31,6 +31,7 @@ public class PPLQueryRequestFactory { private static final String QUERY_PARAMS_PRETTY = "pretty"; private static final String QUERY_PARAMS_PROFILE = "profile"; private static final String QUERY_PARAMS_FETCH_SIZE = "fetch_size"; + private static final String QUERY_PARAMS_INCLUDE_METADATA = "include_metadata"; /** * Build {@link PPLQueryRequest} from {@link RestRequest}. @@ -97,6 +98,13 @@ private static PPLQueryRequest parsePPLRequestFromPayload(RestRequest restReques "Invalid fetch_size parameter: must be a valid integer", e); } } + // Support include_metadata as a URL parameter if not already in the JSON body + if (!jsonContent.has(QUERY_PARAMS_INCLUDE_METADATA) + && restRequest.params().containsKey(QUERY_PARAMS_INCLUDE_METADATA)) { + jsonContent.put( + QUERY_PARAMS_INCLUDE_METADATA, + Boolean.parseBoolean(restRequest.params().get(QUERY_PARAMS_INCLUDE_METADATA))); + } PPLQueryRequest pplRequest = new PPLQueryRequest( jsonContent.getString(PPL_FIELD_NAME), diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java index 5c6266beee1..6c17b555f13 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java @@ -104,7 +104,8 @@ public String getName() { @Override protected Set responseParams() { Set responseParams = new HashSet<>(super.responseParams()); - responseParams.addAll(Arrays.asList("format", "mode", "sanitize", "fetch_size")); + responseParams.addAll( + Arrays.asList("format", "mode", "sanitize", "fetch_size", "include_metadata")); return responseParams; } diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java b/ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java index ea353066cb0..a7973085c3a 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java @@ -91,6 +91,9 @@ private AbstractPlan plan( ResponseListener explainListener) { // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) ParseTree cst = parser.parse(request.getRequest()); + + boolean includeMetadata = request.getIncludeMetadata(); + Statement statement = cst.accept( new AstStatementBuilder( @@ -101,6 +104,7 @@ private AbstractPlan plan( .highlightConfig(request.getHighlightConfig()) .format(request.getFormat()) .explainMode(request.getExplainMode()) + .includeMetadata(includeMetadata) .build())); log.info( diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java b/ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java index 06c7fe1c38e..11b31183088 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java @@ -26,6 +26,7 @@ public class PPLQueryRequest { private static final String DEFAULT_PPL_PATH = "/_plugins/_ppl"; private static final String FETCH_SIZE_FIELD = "fetch_size"; private static final String HIGHLIGHT_FIELD = "highlight"; + private static final String INCLUDE_METADATA_FIELD = "include_metadata"; private static final int MAX_HIGHLIGHT_FIELDS = 100; private static final int MAX_TAG_ENTRIES = 10; @@ -124,6 +125,20 @@ public int getFetchSize() { return jsonContent.optInt(FETCH_SIZE_FIELD, 0); } + /** + * Get whether to include metadata fields (_id, _index, _score, etc.) in the response. When + * enabled, metadata fields will be included alongside regular fields in wildcard field selections + * (e.g., fields *). + * + * @return true if metadata fields should be included, false otherwise (default: false) + */ + public boolean getIncludeMetadata() { + if (jsonContent == null) { + return false; + } + return jsonContent.optBoolean(INCLUDE_METADATA_FIELD, false); + } + /** * Get highlight config from the request. Supports both the simple array format ({@code ["*"]}) * and the rich OSD object format with {@code pre_tags}, {@code post_tags}, {@code fields}, and diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java index d2c1f610238..237e422b080 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java @@ -12,6 +12,7 @@ import lombok.Data; import lombok.RequiredArgsConstructor; import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.statement.Explain; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; @@ -37,7 +38,7 @@ public Statement visitPplStatement(OpenSearchPPLParser.PplStatementContext ctx) rawPlan = new Head(context.getFetchSize(), 0).attach(rawPlan); } UnresolvedPlan plan = addSelectAll(rawPlan); - Query query = new Query(plan, 0, PPL); + Query query = new Query(plan, 0, PPL, context.isIncludeMetadata()); if (context.getHighlightConfig() != null && context.getHighlightConfig().fields() != null && !context.getHighlightConfig().fields().isEmpty()) { @@ -76,13 +77,21 @@ public static class StatementBuilderContext { private final String format; private final String explainMode; + + /** Whether to include metadata fields like _id, _index, _score in the result. */ + private final boolean includeMetadata; } private UnresolvedPlan addSelectAll(UnresolvedPlan plan) { if ((plan instanceof Project) && !((Project) plan).isExcluded()) { return plan; } else { - return new Project(ImmutableList.of(AllFields.of())).attach(plan); + // Use AllFieldsExcludeMeta when include_metadata is false (default behavior) + // Use AllFields when include_metadata is true (include metadata fields) + boolean includeMetadata = context.isIncludeMetadata(); + AllFields allFields = includeMetadata ? AllFields.of() : AllFieldsExcludeMeta.of(); + + return new Project(ImmutableList.of(allFields)).attach(plan); } } } diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelper.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelper.java index a67507be315..8072433b7c4 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelper.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelper.java @@ -7,7 +7,7 @@ import com.google.common.collect.ImmutableList; import lombok.experimental.UtilityClass; -import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.common.setting.Settings; @@ -21,7 +21,7 @@ public UnresolvedPlan addSelectAll(UnresolvedPlan plan) { if ((plan instanceof Project) && !((Project) plan).isExcluded()) { return plan; } else { - return new Project(ImmutableList.of(AllFields.of())).attach(plan); + return new Project(ImmutableList.of(AllFieldsExcludeMeta.of())).attach(plan); } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java index 0825f6d1def..2d9a099023a 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java @@ -100,15 +100,6 @@ public void onFailure(Exception e) { @Test public void testExecuteShouldPass() { - doAnswer( - invocation -> { - ResponseListener listener = invocation.getArgument(3); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); - return null; - }) - .when(queryService) - .execute(any(), any(), any(), any()); - pplService.execute( new PPLQueryRequest("search source=t a=1", null, QUERY), getQueryListener(false), @@ -117,15 +108,6 @@ public void testExecuteShouldPass() { @Test public void testExecuteCsvFormatShouldPass() { - doAnswer( - invocation -> { - ResponseListener listener = invocation.getArgument(3); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); - return null; - }) - .when(queryService) - .execute(any(), any(), any(), any()); - pplService.execute( new PPLQueryRequest("search source=t a=1", null, QUERY, "csv"), getQueryListener(false), @@ -134,15 +116,6 @@ public void testExecuteCsvFormatShouldPass() { @Test public void testExplainShouldPass() { - doAnswer( - invocation -> { - ResponseListener listener = invocation.getArgument(3); - listener.onResponse(new ExplainResponse(new ExplainResponseNode("test"))); - return null; - }) - .when(queryService) - .explain(any(), any(), any(), any(), any()); - pplService.explain( new PPLQueryRequest("search source=t a=1", null, EXPLAIN), new ResponseListener() { @@ -171,15 +144,6 @@ public void testExplainWithIllegalQueryShouldBeCaughtByHandler() { @Test public void testPrometheusQuery() { - doAnswer( - invocation -> { - ResponseListener listener = invocation.getArgument(3); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); - return null; - }) - .when(queryService) - .execute(any(), any(), any(), any()); - pplService.execute( new PPLQueryRequest("source = prometheus.http_requests_total", null, QUERY), getQueryListener(false), diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFlattenTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFlattenTest.java index 0cb3a90e557..7c8df8b7195 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFlattenTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLFlattenTest.java @@ -64,16 +64,16 @@ public void testFlatten() { RelNode root = getRelNode(ppl); // Regarded as an identity scan. See RelBuilder#L2801 String expectedLogical = - "LogicalProject(DEPTNO=[$0], EMP=[$1], EMPNAME=[$2], EMPNO=[$3])\n" + "LogicalProject(DEPTNO=[$0], EMP=[$1], EMP.EMPNAME=[$2], EMP.EMPNO=[$3], EMPNAME=[$2], EMPNO=[$3])\n" + " LogicalTableScan(table=[[scott, DEPT]])\n"; verifyLogical(root, expectedLogical); String expectedSparkSql = - "SELECT `DEPTNO`, `EMP`, `EMP.EMPNAME` `EMPNAME`, `EMP.EMPNO` `EMPNO`\nFROM `scott`.`DEPT`"; + "SELECT `DEPTNO`, `EMP`, `EMP.EMPNAME`, `EMP.EMPNO`, `EMP.EMPNAME` `EMPNAME`, `EMP.EMPNO` `EMPNO`\nFROM `scott`.`DEPT`"; verifyPPLToSparkSQL(root, expectedSparkSql); String expectedResult = - "DEPTNO=10; EMP={7369, SMITH}; EMPNAME=SMITH; EMPNO=7369\n" - + "DEPTNO=20; EMP={7499, ALLEN}; EMPNAME=ALLEN; EMPNO=7499\n" - + "DEPTNO=30; EMP={7521, WARD}; EMPNAME=WARD; EMPNO=7521\n"; + "DEPTNO=10; EMP={7369, SMITH}; EMP.EMPNAME=SMITH; EMP.EMPNO=7369; EMPNAME=SMITH; EMPNO=7369\n" + + "DEPTNO=20; EMP={7499, ALLEN}; EMP.EMPNAME=ALLEN; EMP.EMPNO=7499; EMPNAME=ALLEN; EMPNO=7499\n" + + "DEPTNO=30; EMP={7521, WARD}; EMP.EMPNAME=WARD; EMP.EMPNO=7521; EMPNAME=WARD; EMPNO=7521\n"; verifyResult(root, expectedResult); } @@ -82,16 +82,16 @@ public void testFlattenWithAliases() { String ppl = "source=DEPT | flatten EMP as name, number"; RelNode root = getRelNode(ppl); String expectedLogical = - "LogicalProject(DEPTNO=[$0], EMP=[$1], name=[$2], number=[$3])\n" + "LogicalProject(DEPTNO=[$0], EMP=[$1], EMP.EMPNAME=[$2], EMP.EMPNO=[$3], name=[$2], number=[$3])\n" + " LogicalTableScan(table=[[scott, DEPT]])\n"; verifyLogical(root, expectedLogical); String expectedSparkSql = - "SELECT `DEPTNO`, `EMP`, `EMP.EMPNAME` `name`, `EMP.EMPNO` `number`\nFROM `scott`.`DEPT`"; + "SELECT `DEPTNO`, `EMP`, `EMP.EMPNAME`, `EMP.EMPNO`, `EMP.EMPNAME` `name`, `EMP.EMPNO` `number`\nFROM `scott`.`DEPT`"; verifyPPLToSparkSQL(root, expectedSparkSql); String expectedResult = - "DEPTNO=10; EMP={7369, SMITH}; name=SMITH; number=7369\n" - + "DEPTNO=20; EMP={7499, ALLEN}; name=ALLEN; number=7499\n" - + "DEPTNO=30; EMP={7521, WARD}; name=WARD; number=7521\n"; + "DEPTNO=10; EMP={7369, SMITH}; EMP.EMPNAME=SMITH; EMP.EMPNO=7369; name=SMITH; number=7369\n" + + "DEPTNO=20; EMP={7499, ALLEN}; EMP.EMPNAME=ALLEN; EMP.EMPNO=7499; name=ALLEN; number=7499\n" + + "DEPTNO=30; EMP={7521, WARD}; EMP.EMPNAME=WARD; EMP.EMPNO=7521; name=WARD; number=7521\n"; verifyResult(root, expectedResult); } @@ -104,12 +104,12 @@ public void testProject() { String ppl = "source=DEPT"; RelNode root = getRelNode(ppl); String expectedLogical = - "LogicalProject(DEPTNO=[$0], EMP=[$1])\n LogicalTableScan(table=[[scott, DEPT]])\n"; + "LogicalTableScan(table=[[scott, DEPT]])\n"; verifyLogical(root, expectedLogical); String expectedResult = - "DEPTNO=10; EMP={7369, SMITH}\n" - + "DEPTNO=20; EMP={7499, ALLEN}\n" - + "DEPTNO=30; EMP={7521, WARD}\n"; + "DEPTNO=10; EMP={7369, SMITH}; EMP.EMPNAME=SMITH; EMP.EMPNO=7369\n" + + "DEPTNO=20; EMP={7499, ALLEN}; EMP.EMPNAME=ALLEN; EMP.EMPNO=7499\n" + + "DEPTNO=30; EMP={7521, WARD}; EMP.EMPNAME=WARD; EMP.EMPNO=7521\n"; verifyResult(root, expectedResult); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLSpathTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLSpathTest.java index 9967b10543e..879d48bc4de 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLSpathTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLSpathTest.java @@ -123,6 +123,34 @@ public void testSpathAutoExtractModeWithFields() { + "FROM `scott`.`EMP`"); } + @Test + public void testSpathAutoExtractWithMultiFieldEval() { + // Issue #5185: eval with multiple dotted-path assignments from MAP column + // should not remove the MAP root field + withPPLQuery( + "source=EMP | spath input=ENAME" + + " | eval ENAME.user.name=ENAME.user.name, ENAME.user.age=ENAME.user.age" + + " | fields ENAME.user.name, ENAME.user.age") + .expectLogical( + "LogicalProject(ENAME.user.name=[ITEM(JSON_EXTRACT_ALL($1), 'user.name')]," + + " ENAME.user.age=[ITEM(JSON_EXTRACT_ALL($1), 'user.age')])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"); + } + + @Test + public void testSpathAutoExtractWithSeparateEvalCommands() { + // Issue #5185: separate eval commands with dotted-path assignments from MAP column + withPPLQuery( + "source=EMP | spath input=ENAME" + + " | eval ENAME.user.name=ENAME.user.name" + + " | eval ENAME.user.age=ENAME.user.age" + + " | fields ENAME.user.name, ENAME.user.age") + .expectLogical( + "LogicalProject(ENAME.user.name=[ITEM(JSON_EXTRACT_ALL($1), 'user.name')]," + + " ENAME.user.age=[ITEM(JSON_EXTRACT_ALL($1), 'user.age')])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"); + } + @Test public void testSpathAutoExtractModeWithSort() { withPPLQuery("source=EMP | spath input=ENAME output=result" + " | sort result.user.name") diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstStatementBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstStatementBuilderTest.java index ba31b75b38b..bcf39383b33 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstStatementBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstStatementBuilderTest.java @@ -16,6 +16,7 @@ import org.mockito.Mock; import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.statement.Explain; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; @@ -36,12 +37,13 @@ public void buildQueryStatement() { assertEqual( "search source=t | where a=1", new Query( - project(filter(relation("t"), compare("=", field("a"), intLiteral(1))), AllFields.of()), + project(filter(relation("t"), compare("=", field("a"), intLiteral(1))), AllFieldsExcludeMeta.of()), 0, - PPL)); + PPL, + false)); assertEqual( "search source=t a=1", - new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL)); + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test @@ -51,13 +53,15 @@ public void buildExplainStatement() { new Explain( new Query( project( - filter(relation("t"), compare("=", field("a"), intLiteral(1))), AllFields.of()), + filter(relation("t"), compare("=", field("a"), intLiteral(1))), AllFieldsExcludeMeta.of()), 0, - PPL), + PPL, + false), PPL)); assertExplainEqual( "search source=t a=1", - new Explain(new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL), PPL)); + new Explain( + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false), PPL)); } @Test @@ -66,7 +70,8 @@ public void buildQueryStatementWithFetchSize() { assertEqualWithFetchSize( "search source=t a=1", 100, - new Query(project(head(search(relation("t"), "a:1"), 100, 0), AllFields.of()), 0, PPL)); + new Query( + project(head(search(relation("t"), "a:1"), 100, 0), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test @@ -75,7 +80,7 @@ public void buildQueryStatementWithFetchSizeZero() { assertEqualWithFetchSize( "search source=t a=1", 0, - new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL)); + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test @@ -83,7 +88,8 @@ public void buildQueryStatementWithLargeFetchSize() { assertEqualWithFetchSize( "search source=t a=1", 10000, - new Query(project(head(search(relation("t"), "a:1"), 10000, 0), AllFields.of()), 0, PPL)); + new Query( + project(head(search(relation("t"), "a:1"), 10000, 0), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test @@ -94,8 +100,7 @@ public void buildQueryStatementWithFetchSizeAndSmallerHead() { assertEqualWithFetchSize( "source=t | head 3", 10, - new Query(project(head(head(relation("t"), 3, 0), 10, 0), AllFields.of()), 0, PPL)); - } + new Query(project(head(head(relation("t"), 3, 0), 10, 0), AllFieldsExcludeMeta.of()), 0, PPL, false)); @Test public void buildQueryStatementWithFetchSizeSmallerThanHead() { @@ -105,7 +110,7 @@ public void buildQueryStatementWithFetchSizeSmallerThanHead() { assertEqualWithFetchSize( "source=t | head 100", 5, - new Query(project(head(head(relation("t"), 100, 0), 5, 0), AllFields.of()), 0, PPL)); + new Query(project(head(head(relation("t"), 100, 0), 5, 0), AllFields.of()), 0, PPL, false)); } @Test @@ -115,14 +120,15 @@ public void buildQueryStatementWithFetchSizeAndHeadWithOffset() { assertEqualWithFetchSize( "source=t | head 3 from 1", 10, - new Query(project(head(head(relation("t"), 3, 1), 10, 0), AllFields.of()), 0, PPL)); + new Query(project(head(head(relation("t"), 3, 1), 10, 0), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test public void buildQueryStatementWithHighlight() { // Highlight config is set on the Query statement, not as an AST wrapper HighlightConfig config = new HighlightConfig(List.of("*")); - Query expected = new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL); + Query expected = + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false); expected.setHighlightConfig(config); assertEqualWithHighlight("search source=t a=1", config, expected); } @@ -130,7 +136,8 @@ public void buildQueryStatementWithHighlight() { @Test public void buildQueryStatementWithHighlightMultipleTerms() { HighlightConfig config = new HighlightConfig(List.of("error", "login")); - Query expected = new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL); + Query expected = + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false); expected.setHighlightConfig(config); assertEqualWithHighlight("search source=t a=1", config, expected); } @@ -141,7 +148,7 @@ public void buildQueryStatementWithHighlightNull() { assertEqualWithHighlight( "search source=t a=1", null, - new Query(project(search(relation("t"), "a:1"), AllFields.of()), 0, PPL)); + new Query(project(search(relation("t"), "a:1"), AllFieldsExcludeMeta.of()), 0, PPL, false)); } @Test @@ -149,7 +156,8 @@ public void buildQueryStatementWithHighlightAndFetchSize() { // Both fetch_size and highlight: Head wraps the plan, config is on the Query HighlightConfig config = new HighlightConfig(List.of("*")); Query expected = - new Query(project(head(search(relation("t"), "a:1"), 100, 0), AllFields.of()), 0, PPL); + new Query( + project(head(search(relation("t"), "a:1"), 100, 0), AllFieldsExcludeMeta.of()), 0, PPL, false); expected.setHighlightConfig(config); assertEqualWithHighlightAndFetchSize("search source=t a=1", config, 100, expected); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelperTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelperTest.java index 7c1264e0b63..c9aca678e67 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelperTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/utils/UnresolvedPlanHelperTest.java @@ -15,7 +15,7 @@ import org.junit.runner.RunWith; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.expression.UnresolvedExpression; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.ast.tree.Rename; @@ -39,7 +39,7 @@ public void addProjectForProjectExcludeOperator() { UnresolvedPlan plan = UnresolvedPlanHelper.addSelectAll(project); assertTrue(plan instanceof Project); - assertThat(((Project) plan).getProjectList(), Matchers.contains(AllFields.of())); + assertThat(((Project) plan).getProjectList(), Matchers.contains(AllFieldsExcludeMeta.of())); } @Test diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java index dfb045f345a..a76c4368fd2 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java @@ -25,7 +25,7 @@ public class AstStatementBuilder extends OpenSearchSQLParserBaseVisitor