diff --git a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java index 6f432ce7bc7..3bdd317af51 100644 --- a/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java @@ -8,6 +8,7 @@ import org.opensearch.sql.ast.expression.AggregateFunction; import org.opensearch.sql.ast.expression.Alias; import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.expression.And; import org.opensearch.sql.ast.expression.Argument; import org.opensearch.sql.ast.expression.AttributeList; @@ -254,6 +255,10 @@ public T visitAllFields(AllFields node, C context) { return visitChildren(node, context); } + public T visitAllFieldsExcludeMeta(AllFieldsExcludeMeta node, C context) { + return visitChildren(node, context); + } + public T visitNestedAllTupleFields(NestedAllTupleFields node, C context) { return visitChildren(node, context); } diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Alias.java b/core/src/main/java/org/opensearch/sql/ast/expression/Alias.java index d8522b0fc1b..ffe07d6c0b5 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Alias.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Alias.java @@ -5,12 +5,11 @@ package org.opensearch.sql.ast.expression; -import lombok.AllArgsConstructor; import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.RequiredArgsConstructor; import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; /** * Alias abstraction that associate an unnamed expression with a name. The name information @@ -18,10 +17,8 @@ * restoring the info in toString() method which is inaccurate because original info is already * lost. */ -@AllArgsConstructor @EqualsAndHashCode(callSuper = false) @Getter -@RequiredArgsConstructor @ToString public class Alias extends UnresolvedExpression { @@ -35,7 +32,41 @@ public class Alias extends UnresolvedExpression { private final UnresolvedExpression delegated; /** TODO. Optional field alias. This field is OpenSearch SQL-only */ - private String alias; + private final String alias; + + public Alias(String name, UnresolvedExpression expr) { + this(name, expr, false); + } + + public Alias(String name, UnresolvedExpression expr, String alias) { + this(name, expr, alias, false); + } + + public Alias(String name, UnresolvedExpression expr, boolean metaDataFieldAllowed) { + this(name, expr, null, metaDataFieldAllowed); + } + + /** + * @param metadataFieldAllowed Whether do we allow metadata field as alias name. Should Only be + * true for SQL, see {@link Alias::newAliasAllowMetaMetaField} + */ + private Alias( + String name, UnresolvedExpression expr, String alias, boolean metadataFieldAllowed) { + if (!metadataFieldAllowed && OpenSearchConstants.METADATAFIELD_TYPE_MAP.containsKey(name)) { + throw new IllegalArgumentException( + String.format("Cannot use metadata field [%s] as the alias.", name)); + } + this.name = name; + this.delegated = expr; + this.alias = alias; + } + + // TODO: Only for SQL. We never allow metadata field as alias but SQL view all select items as + // alias. Need to remove this tricky logic after SQL fix it. + public static Alias newAliasAllowMetaMetaField( + String name, UnresolvedExpression expr, String alias) { + return new Alias(name, expr, alias, true); + } @Override public T accept(AbstractNodeVisitor nodeVisitor, C context) { diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/AllFields.java b/core/src/main/java/org/opensearch/sql/ast/expression/AllFields.java index b9b90ea24a8..1ac66b287c7 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/AllFields.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/AllFields.java @@ -18,7 +18,7 @@ public class AllFields extends UnresolvedExpression { public static final AllFields INSTANCE = new AllFields(); - private AllFields() {} + public AllFields() {} public static AllFields of() { return INSTANCE; diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/AllFieldsExcludeMeta.java b/core/src/main/java/org/opensearch/sql/ast/expression/AllFieldsExcludeMeta.java new file mode 100644 index 00000000000..f85d55e595e --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/ast/expression/AllFieldsExcludeMeta.java @@ -0,0 +1,41 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ast.expression; + +import java.util.Collections; +import java.util.List; +import lombok.EqualsAndHashCode; +import lombok.ToString; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; + +/** + * Represent the All fields but excluding metadata fields if user never uses them in the previous + * fields command + */ +@ToString +@EqualsAndHashCode(callSuper = false) +public class AllFieldsExcludeMeta extends AllFields { + public static final AllFieldsExcludeMeta INSTANCE = new AllFieldsExcludeMeta(); + + private AllFieldsExcludeMeta() { + super(); + } + + public static AllFieldsExcludeMeta of() { + return INSTANCE; + } + + @Override + public List getChild() { + return Collections.emptyList(); + } + + @Override + public R accept(AbstractNodeVisitor nodeVisitor, C context) { + return nodeVisitor.visitAllFieldsExcludeMeta(this, context); + } +} diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Let.java b/core/src/main/java/org/opensearch/sql/ast/expression/Let.java index 2f63a25f106..ad9843fae11 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Let.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Let.java @@ -9,19 +9,28 @@ import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.RequiredArgsConstructor; import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; /** Represent the assign operation. e.g. velocity = distance/speed. */ @Getter @ToString @EqualsAndHashCode(callSuper = false) -@RequiredArgsConstructor public class Let extends UnresolvedExpression { private final Field var; private final UnresolvedExpression expression; + public Let(Field var, UnresolvedExpression expression) { + String varName = var.getField().toString(); + if (OpenSearchConstants.METADATAFIELD_TYPE_MAP.containsKey(varName)) { + throw new IllegalArgumentException( + String.format("Cannot use metadata field [%s] as the eval field.", varName)); + } + this.var = var; + this.expression = expression; + } + @Override public List getChild() { return ImmutableList.of(); diff --git a/core/src/main/java/org/opensearch/sql/ast/tree/Rename.java b/core/src/main/java/org/opensearch/sql/ast/tree/Rename.java index e6f760aca00..a837a1835ac 100644 --- a/core/src/main/java/org/opensearch/sql/ast/tree/Rename.java +++ b/core/src/main/java/org/opensearch/sql/ast/tree/Rename.java @@ -9,15 +9,16 @@ import java.util.List; import lombok.EqualsAndHashCode; import lombok.Getter; -import lombok.RequiredArgsConstructor; import lombok.ToString; import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Map; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; @ToString @EqualsAndHashCode(callSuper = false) @Getter -@RequiredArgsConstructor public class Rename extends UnresolvedPlan { private final List renameList; private UnresolvedPlan child; @@ -25,6 +26,26 @@ public class Rename extends UnresolvedPlan { public Rename(List renameList, UnresolvedPlan child) { this.renameList = renameList; this.child = child; + validate(); + } + + public Rename(List renameList) { + this.renameList = renameList; + validate(); + } + + private void validate() { + renameList.forEach(rename -> validate(rename.getTarget())); + } + + private void validate(UnresolvedExpression expr) { + if (expr instanceof Field field) { + String name = field.getField().toString(); + if (OpenSearchConstants.METADATAFIELD_TYPE_MAP.containsKey(name)) { + throw new IllegalArgumentException( + String.format("Cannot use metadata field [%s] in Rename command.", name)); + } + } } @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 5e6d81a9f30..189db7d03b1 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java @@ -32,7 +32,16 @@ public class CalcitePlanContext { public final QueryType queryType; @Getter @Setter private boolean isResolvingJoinCondition = false; - @Getter @Setter private boolean isResolvingExistsSubquery = false; + @Getter @Setter private boolean isResolvingSubquery = false; + + /** + * The flag used to determine whether we do metadata field projection for user 1. If a project is + * never visited, we will do metadata field projection for user 2. Else not because user may + * intend to show the metadata field themselves. // TODO: use stack here if we want to do similar + * projection for subquery. + */ + @Getter @Setter private boolean isProjectVisited = false; + private final Stack correlVar = new Stack<>(); private CalcitePlanContext(FrameworkConfig config, QueryType queryType) { 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 e347d04b1c3..e170d7c9cce 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java @@ -40,6 +40,7 @@ import org.opensearch.sql.ast.AbstractNodeVisitor; 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.expression.Argument; import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Let; @@ -73,6 +74,7 @@ import org.opensearch.sql.ast.tree.TableFunction; import org.opensearch.sql.ast.tree.Trendline; import org.opensearch.sql.ast.tree.UnresolvedPlan; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.calcite.utils.JoinAndLookupUtils; import org.opensearch.sql.exception.CalciteUnsupportedException; import org.opensearch.sql.exception.SemanticCheckException; @@ -150,8 +152,10 @@ private boolean containsSubqueryExpression(Node expr) { public RelNode visitProject(Project node, CalcitePlanContext context) { visitChildren(node, context); List projectList; - if (node.getProjectList().stream().anyMatch(e -> e instanceof AllFields)) { + if (node.getProjectList().size() == 1 + && node.getProjectList().getFirst() instanceof AllFields allFields) { tryToRemoveNestedFields(context); + tryToRemoveMetaFields(context, allFields instanceof AllFieldsExcludeMeta); return context.relBuilder.peek(); } else { projectList = @@ -162,6 +166,10 @@ public RelNode visitProject(Project node, CalcitePlanContext context) { if (node.isExcluded()) { context.relBuilder.projectExcept(projectList); } else { + // Only set when not resolving subquery and it's not projectExcept. + if (!context.isResolvingSubquery()) { + context.setProjectVisited(true); + } context.relBuilder.project(projectList); } return context.relBuilder.peek(); @@ -184,6 +192,31 @@ private void tryToRemoveNestedFields(CalcitePlanContext context) { } } + /** + * Try to remove metadata fields in two cases: + * + *

1. It's explicitly specified excluding by force, usually for join or subquery. + * + *

2. There is no other project ever visited in the main query + * + * @param context CalcitePlanContext + * @param excludeByForce whether exclude metadata fields by force + */ + private static void tryToRemoveMetaFields(CalcitePlanContext context, boolean excludeByForce) { + if (excludeByForce || !context.isProjectVisited()) { + List originalFields = context.relBuilder.peek().getRowType().getFieldNames(); + List metaFieldsRef = + originalFields.stream() + .filter(OpenSearchConstants.METADATAFIELD_TYPE_MAP::containsKey) + .map(metaField -> (RexNode) context.relBuilder.field(metaField)) + .toList(); + // Remove metadata fields if there is and ensure there are other fields. + if (!metaFieldsRef.isEmpty() && metaFieldsRef.size() != originalFields.size()) { + context.relBuilder.projectExcept(metaFieldsRef); + } + } + } + @Override public RelNode visitRename(Rename node, CalcitePlanContext context) { visitChildren(node, context); diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index 4de88015fcb..d7c37c55b81 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -398,6 +398,8 @@ public RexNode visitExistsSubquery(ExistsSubquery node, CalcitePlanContext conte } private RelNode resolveSubqueryPlan(UnresolvedPlan subquery, CalcitePlanContext context) { + boolean isNestedSubquery = context.isResolvingSubquery(); + context.setResolvingSubquery(true); // clear and store the outer state boolean isResolvingJoinConditionOuter = context.isResolvingJoinCondition(); if (isResolvingJoinConditionOuter) { @@ -411,6 +413,10 @@ private RelNode resolveSubqueryPlan(UnresolvedPlan subquery, CalcitePlanContext if (isResolvingJoinConditionOuter) { context.setResolvingJoinCondition(true); } + // Only need to set isResolvingSubquery to false if it's not nested subquery. + if (!isNestedSubquery) { + context.setResolvingSubquery(false); + } return subqueryRel; } diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java index 0690c1f5a36..62a8e0f00d0 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java @@ -311,7 +311,9 @@ public static ExprValue getExprValueByExprType(ExprType type, Object value) { public static RelDataType convertSchema(Table table) { List fieldNameList = new ArrayList<>(); List typeList = new ArrayList<>(); - for (Entry entry : table.getFieldTypes().entrySet()) { + Map fieldTypes = table.getFieldTypes(); + fieldTypes.putAll(table.getReservedFieldTypes()); + for (Entry entry : fieldTypes.entrySet()) { fieldNameList.add(entry.getKey()); typeList.add(OpenSearchTypeFactory.convertExprTypeToRelDataType(entry.getValue())); } diff --git a/docs/dev/intro-v3-engine.md b/docs/dev/intro-v3-engine.md index ecd4f856a8b..9b5d8d1e70d 100644 --- a/docs/dev/intro-v3-engine.md +++ b/docs/dev/intro-v3-engine.md @@ -55,6 +55,13 @@ As v3 engine is experimental in 3.0.0-beta, not all PPL commands could work unde ### 3.3 Limitations +For the following commands or functions, we add some defensive restrictions to ensure security. + +#### New Restrictions +- `EVAL` won't allow to use [Metadata Fields of OpenSearch](https://docs.opensearch.org/docs/latest/field-types/metadata-fields/index/) as the fields +- `RENAME` won't allow renaming to a [Metadata Fields of OpenSearch](https://docs.opensearch.org/docs/latest/field-types/metadata-fields/index/) +- `as` won't allow to use [Metadata Fields of OpenSearch](https://docs.opensearch.org/docs/latest/field-types/metadata-fields/index/) as the alias name + For the following functionalities in V3 engine, the query will be forwarded to the V2 query engine and thus you cannot use new features in [2. What's New](#2-whats-new). #### Unsupported functionalities @@ -101,4 +108,3 @@ The following items are on our roadmap with high priority: - Backport to 2.19.x - Unified the PPL syntax between [PPL-on-OpenSearch](https://github.com/opensearch-project/sql/blob/main/ppl/src/main/antlr/OpenSearchPPLParser.g4) and [PPL-on-Spark](https://github.com/opensearch-project/opensearch-spark/blob/main/ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4) - Support more DSL aggregation - diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteFieldsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteFieldsCommandIT.java index 63402667fae..0de2e28eb31 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteFieldsCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteFieldsCommandIT.java @@ -5,7 +5,6 @@ package org.opensearch.sql.calcite.remote; -import java.io.IOException; import org.opensearch.sql.ppl.FieldsCommandIT; public class CalciteFieldsCommandIT extends FieldsCommandIT { @@ -15,30 +14,4 @@ public void init() throws Exception { enableCalcite(); disallowCalciteFallback(); } - - @Override - public void testDelimitedMetadataFields() throws IOException { - withFallbackEnabled( - () -> { - try { - super.testDelimitedMetadataFields(); - } catch (IOException e) { - throw new RuntimeException(e); - } - }, - "Calcite doesn't support metadata fields in fields yet"); - } - - @Override - public void testMetadataFields() throws IOException { - withFallbackEnabled( - () -> { - try { - super.testMetadataFields(); - } catch (IOException e) { - throw new RuntimeException(e); - } - }, - "Calcite doesn't support metadata fields in fields yet"); - } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteWhereCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteWhereCommandIT.java index a59399cac93..5c07f06bbc9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteWhereCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteWhereCommandIT.java @@ -6,7 +6,6 @@ package org.opensearch.sql.calcite.remote; import java.io.IOException; -import lombok.SneakyThrows; import org.opensearch.sql.ppl.WhereCommandIT; public class CalciteWhereCommandIT extends WhereCommandIT { @@ -30,20 +29,6 @@ public void testIsNotNullFunction() throws IOException { "https://github.com/opensearch-project/sql/issues/3428"); } - @SneakyThrows - @Override - public void testWhereWithMetadataFields() { - withFallbackEnabled( - () -> { - try { - super.testWhereWithMetadataFields(); - } catch (IOException e) { - throw new RuntimeException(e); - } - }, - "https://github.com/opensearch-poject/sql/issues/3333"); - } - @Override protected String getIncompatibleTypeErrMsg() { return "In expression types are incompatible: fields type LONG, values type [INTEGER, INTEGER," diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java index 70a220a301f..4329b1dbd66 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBasicIT.java @@ -5,6 +5,7 @@ package org.opensearch.sql.calcite.standalone; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ALIAS; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; import static org.opensearch.sql.util.MatcherUtils.rows; @@ -90,7 +91,10 @@ public void testSourceFieldQuery() { public void testFieldsShouldBeCaseSensitive() { IllegalStateException e = assertThrows(IllegalStateException.class, () -> execute("source=test | fields NAME")); - verifyErrorMessageContains(e, "field [NAME] not found; input fields are: [name, age]"); + verifyErrorMessageContains( + e, + "field [NAME] not found; input fields are: [name, age, _id, _index, _score, _maxscore," + + " _sort, _routing]"); } @Test @@ -530,4 +534,15 @@ public void testAliasDataType() { verifySchema(result, schema("original_col", "integer"), schema("alias_col", "integer")); verifyDataRows(result, rows(2, 2), rows(3, 3)); } + + @Test + public void testMetaFieldAlias() { + Exception e = + assertThrows( + Exception.class, + () -> + executeQuery( + String.format("source=%s | stats count() as _score", TEST_INDEX_ACCOUNT))); + verifyErrorMessageContains(e, "Cannot use metadata field [_score] as the alias."); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLInSubqueryIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLInSubqueryIT.java index b59eed2b808..be1d8d2cc6a 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLInSubqueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLInSubqueryIT.java @@ -297,6 +297,39 @@ public void testNestedInSubquery() { rows(1006, "Tommy", 30000)); } + @Test + public void testNestedInSubquery2() { + JSONObject result = + executeQuery( + String.format( + """ + source = %s + | where id in [ + source = %s + | where occupation in [ + source = %s + | where occupation != 'Engineer' + | fields occupation + ] + | fields uid + ] + | sort - salary + """, + TEST_INDEX_WORKER, TEST_INDEX_WORK_INFORMATION, TEST_INDEX_OCCUPATION)); + verifySchema( + result, + schema("name", "string"), + schema("country", "string"), + schema("occupation", "string"), + schema("id", "integer"), + schema("salary", "integer")); + verifyDataRowsInOrder( + result, + rows("John", "Canada", "Doctor", 1002, 120000), + rows("David", null, "Doctor", 1003, 120000), + rows("Tommy", "USA", "Teacher", 1006, 30000)); + } + @Ignore // TODO bug? fail in execution, the plan converted is correct public void testInSubqueryAsJoinFilter() { JSONObject result = diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLRenameIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLRenameIT.java index 878a3c9e8c0..891630500f2 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLRenameIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLRenameIT.java @@ -56,7 +56,8 @@ public void testRefRenamedField() { """, TEST_INDEX_STATE_COUNTRY))); assertEquals( - "field [age] not found; input fields are: [name, country, state, month, year, renamed_age]", + "field [age] not found; input fields are: [name, country, state, month, year, renamed_age," + + " _id, _index, _score, _maxscore, _sort, _routing]", e.getMessage()); } @@ -73,10 +74,43 @@ public void testRenameNotExistedField() { """, TEST_INDEX_STATE_COUNTRY))); assertEquals( - "field [renamed_age] not found; input fields are: [name, country, state, month, year, age]", + "field [renamed_age] not found; input fields are: [name, country, state, month, year, age," + + " _id, _index, _score, _maxscore, _sort, _routing]", e.getMessage()); } + @Test + public void testRenameToMetaField() { + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, + () -> + executeQuery( + String.format( + """ + source = %s | rename name as _id + """, + TEST_INDEX_STATE_COUNTRY))); + assertEquals("Cannot use metadata field [_id] in Rename command.", e.getMessage()); + + // Test rename to _ID, which is allowed as metadata fields name is case-sensitive + JSONObject result = + executeQuery( + String.format( + """ + source = %s | rename age as _ID + """, + TEST_INDEX_STATE_COUNTRY)); + verifySchema( + result, + schema("name", "string"), + schema("_ID", "integer"), + schema("state", "string"), + schema("country", "string"), + schema("year", "integer"), + schema("month", "integer")); + } + @Test public void testMultipleRename() { JSONObject result = diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/FieldsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/FieldsCommandIT.java index 022a1fba036..2589750f617 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/FieldsCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/FieldsCommandIT.java @@ -13,6 +13,7 @@ import static org.opensearch.sql.util.MatcherUtils.schema; import static org.opensearch.sql.util.MatcherUtils.verifyColumn; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifyErrorMessageContains; import static org.opensearch.sql.util.MatcherUtils.verifySchema; import java.io.IOException; @@ -84,4 +85,24 @@ public void testDelimitedMetadataFields() throws IOException { String.format("source=%s | fields firstname, `_id`, `_index`", TEST_INDEX_ACCOUNT)); verifyColumn(result, columnName("firstname"), columnName("_id"), columnName("_index")); } + + @Test + public void testMetadataFieldsWithEval() throws IOException { + JSONObject result = + executeQuery( + String.format("source=%s | eval a = 1 | fields firstname, _index", TEST_INDEX_ACCOUNT)); + verifyColumn(result, columnName("firstname"), columnName("_index")); + } + + @Test + public void testMetadataFieldsWithEvalMetaField() { + Exception e = + assertThrows( + Exception.class, + () -> + executeQuery( + String.format( + "source=%s | eval _id = 1 | fields firstname, _id", TEST_INDEX_ACCOUNT))); + verifyErrorMessageContains(e, "Cannot use metadata field [_id] as the eval field."); + } } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java index 15e3822391e..12f6b9658a6 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java @@ -111,6 +111,26 @@ public void testWhereWithMetadataFields() throws IOException { verifyDataRows(result, rows("Amber")); } + @Test + public void testWhereWithMetadataFields2() throws IOException { + JSONObject result = + executeQuery(String.format("source=%s | where _id='1'", TEST_INDEX_ACCOUNT)); + verifyDataRows( + result, + rows( + 1, + "Amber", + "880 Holmes Lane", + 39225, + "M", + "Brogan", + "Pyrami", + "IL", + 32, + "amberduke@pyrami.com", + "Duke")); + } + @Test public void testWhereWithIn() throws IOException { JSONObject result = diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json index d9bfc5d083c..b6f843c62b1 100644 --- a/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json @@ -1,6 +1,6 @@ { - "calcite": { - "logical": "LogicalProject(age=[$8])\n LogicalFilter(condition=[>($3, 10000)])\n LogicalFilter(condition=[<($8, 40)])\n LogicalFilter(condition=[>($8, 30)])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", - "physical": "EnumerableCalc(expr#0..10=[{inputs}], age=[$t8])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[FILTER->>($8, 30), FILTER->AND(<($8, 40), >($3, 10000))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"bool\":{\"filter\":[{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},{\"bool\":{\"must\":[{\"range\":{\"age\":{\"from\":null,\"to\":40,\"include_lower\":true,\"include_upper\":false,\"boost\":1.0}}},{\"range\":{\"balance\":{\"from\":10000,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}}],\"adjust_pure_negative\":true,\"boost\":1.0}}],\"adjust_pure_negative\":true,\"boost\":1.0}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=10000, pageSize=null, startFrom=0)])\n" + "calcite":{ + "logical":"LogicalProject(age=[$8])\n LogicalFilter(condition=[>($3, 10000)])\n LogicalFilter(condition=[<($8, 40)])\n LogicalFilter(condition=[>($8, 30)])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", + "physical":"EnumerableCalc(expr#0..16=[{inputs}], age=[$t8])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[FILTER->>($8, 30), FILTER->AND(<($8, 40), >($3, 10000))], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"timeout\":\"1m\",\"query\":{\"bool\":{\"filter\":[{\"range\":{\"age\":{\"from\":30,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}},{\"bool\":{\"must\":[{\"range\":{\"age\":{\"from\":null,\"to\":40,\"include_lower\":true,\"include_upper\":false,\"boost\":1.0}}},{\"range\":{\"balance\":{\"from\":10000,\"to\":null,\"include_lower\":false,\"include_upper\":true,\"boost\":1.0}}}],\"adjust_pure_negative\":true,\"boost\":1.0}}],\"adjust_pure_negative\":true,\"boost\":1.0}},\"sort\":[{\"_doc\":{\"order\":\"asc\"}}]}, requestedTotalSize=10000, pageSize=null, startFrom=0)])\n" } -} \ No newline at end of file +} diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_limit_push.json b/integ-test/src/test/resources/expectedOutput/calcite/explain_limit_push.json index d8ce69035f4..7ee6c76cee7 100644 --- a/integ-test/src/test/resources/expectedOutput/calcite/explain_limit_push.json +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_limit_push.json @@ -1,6 +1,6 @@ { - "calcite": { - "logical": "LogicalProject(ageMinus=[$11])\n LogicalSort(fetch=[5])\n LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10], ageMinus=[-($8, 30)])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", - "physical": "EnumerableCalc(expr#0..10=[{inputs}], expr#11=[30], expr#12=[-($t8, $t11)], ageMinus=[$t12])\n EnumerableLimit(fetch=[5])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n" + "calcite":{ + "logical":"LogicalProject(ageMinus=[$17])\n LogicalSort(fetch=[5])\n LogicalProject(account_number=[$0], firstname=[$1], address=[$2], balance=[$3], gender=[$4], city=[$5], employer=[$6], state=[$7], age=[$8], email=[$9], lastname=[$10], _id=[$11], _index=[$12], _score=[$13], _maxscore=[$14], _sort=[$15], _routing=[$16], ageMinus=[-($8, 30)])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", + "physical":"EnumerableCalc(expr#0..16=[{inputs}], expr#17=[30], expr#18=[-($t8, $t17)], ageMinus=[$t18])\n EnumerableLimit(fetch=[5])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n" } -} \ No newline at end of file +} diff --git a/integ-test/src/test/resources/expectedOutput/calcite/explain_sort_push.json b/integ-test/src/test/resources/expectedOutput/calcite/explain_sort_push.json index 0ebddf498f3..dd765eea54d 100644 --- a/integ-test/src/test/resources/expectedOutput/calcite/explain_sort_push.json +++ b/integ-test/src/test/resources/expectedOutput/calcite/explain_sort_push.json @@ -1,6 +1,6 @@ { - "calcite": { - "logical": "LogicalSort(sort0=[$0], dir0=[ASC])\n LogicalProject(age=[$8])\n LogicalFilter(condition=[>($8, 30)])\n LogicalSort(sort0=[$8], dir0=[ASC])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", - "physical": "EnumerableCalc(expr#0..10=[{inputs}], expr#11=[30], expr#12=[>($t8, $t11)], age=[$t8], $condition=[$t12])\n EnumerableSort(sort0=[$8], dir0=[ASC])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n" + "calcite":{ + "logical":"LogicalSort(sort0=[$0], dir0=[ASC])\n LogicalProject(age=[$8])\n LogicalFilter(condition=[>($8, 30)])\n LogicalSort(sort0=[$8], dir0=[ASC])\n CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n", + "physical":"EnumerableCalc(expr#0..16=[{inputs}], expr#17=[30], expr#18=[>($t8, $t17)], age=[$t8], $condition=[$t18])\n EnumerableSort(sort0=[$8], dir0=[ASC])\n CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])\n" } -} \ No newline at end of file +} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index e43777a7408..83e7fd7721c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -155,7 +155,9 @@ private void addHighlightsToBuilder( private void addMetaDataFieldsToBuilder( ImmutableMap.Builder builder, SearchHit hit) { List metaDataFieldSet = - includes.stream().filter(METADATAFIELD_TYPE_MAP::containsKey).collect(Collectors.toList()); + includes.isEmpty() + ? METADATAFIELD_TYPE_MAP.keySet().stream().toList() + : includes.stream().filter(METADATAFIELD_TYPE_MAP::containsKey).toList(); ExprFloatValue maxScore = Float.isNaN(hits.getMaxScore()) ? null : new ExprFloatValue(hits.getMaxScore()); @@ -176,7 +178,9 @@ private void addMetaDataFieldsToBuilder( } else if (metaDataField.equals(METADATA_FIELD_SORT)) { builder.put(METADATA_FIELD_SORT, new ExprLongValue(hit.getSeqNo())); } else { // if (metaDataField.equals(METADATA_FIELD_ROUTING)){ - builder.put(METADATA_FIELD_ROUTING, new ExprStringValue(hit.getShard().toString())); + builder.put( + METADATA_FIELD_ROUTING, + new ExprStringValue(hit.getShard() == null ? null : hit.getShard().toString())); } }); } 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 8a0a212d168..31ad0b08974 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 @@ -61,13 +61,16 @@ public class OpenSearchIndex extends OpenSearchTable { public static final String METADATA_FIELD_ROUTING = "_routing"; public static final java.util.Map METADATAFIELD_TYPE_MAP = - Map.of( - METADATA_FIELD_ID, ExprCoreType.STRING, - METADATA_FIELD_INDEX, ExprCoreType.STRING, - METADATA_FIELD_SCORE, ExprCoreType.FLOAT, - METADATA_FIELD_MAXSCORE, ExprCoreType.FLOAT, - METADATA_FIELD_SORT, ExprCoreType.LONG, - METADATA_FIELD_ROUTING, ExprCoreType.STRING); + new LinkedHashMap<>() { + { + put(METADATA_FIELD_ID, ExprCoreType.STRING); + put(METADATA_FIELD_INDEX, ExprCoreType.STRING); + put(METADATA_FIELD_SCORE, ExprCoreType.FLOAT); + put(METADATA_FIELD_MAXSCORE, ExprCoreType.FLOAT); + put(METADATA_FIELD_SORT, ExprCoreType.LONG); + put(METADATA_FIELD_ROUTING, ExprCoreType.STRING); + } + }; /** OpenSearch client connection. */ @Getter private final OpenSearchClient client; diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java index 984c98f8033..95fc07a28e3 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java @@ -263,7 +263,7 @@ void iterator_with_inner_hits() { when(factory.construct(any(), anyBoolean())).thenReturn(exprTupleValue1); - for (ExprValue hit : new OpenSearchResponse(searchResponse, factory, includes)) { + for (ExprValue hit : new OpenSearchResponse(searchResponse, factory, List.of("id1"))) { assertEquals(exprTupleValue1, hit); } } diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index e44b5752c05..bc8e89387dc 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java @@ -40,6 +40,7 @@ import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.expression.Argument; import org.opensearch.sql.ast.expression.EqualTo; import org.opensearch.sql.ast.expression.Field; @@ -114,13 +115,17 @@ public AstBuilder(String query, Settings settings) { @Override public UnresolvedPlan visitQueryStatement(OpenSearchPPLParser.QueryStatementContext ctx) { UnresolvedPlan pplCommand = visit(ctx.pplCommands()); - return ctx.commands().stream().map(this::visit).reduce(pplCommand, (r, e) -> e.attach(r)); + return ctx.commands().stream() + .map(this::visit) + .reduce(pplCommand, (r, e) -> e.attach(e instanceof Join ? projectExceptMeta(r) : r)); } @Override public UnresolvedPlan visitSubSearch(OpenSearchPPLParser.SubSearchContext ctx) { UnresolvedPlan searchCommand = visit(ctx.searchCommand()); - return ctx.commands().stream().map(this::visit).reduce(searchCommand, (r, e) -> e.attach(r)); + // Exclude metadata fields for subquery + return projectExceptMeta( + ctx.commands().stream().map(this::visit).reduce(searchCommand, (r, e) -> e.attach(r))); } /** Search command. */ @@ -205,8 +210,8 @@ public UnresolvedPlan visitJoinCommand(OpenSearchPPLParser.JoinCommandContext ct ctx.joinCriteria() == null ? Optional.empty() : Optional.of(expressionBuilder.visitJoinCriteria(ctx.joinCriteria())); - - return new Join(right, leftAlias, rightAlias, joinType, joinCondition, joinHint); + return new Join( + projectExceptMeta(right), leftAlias, rightAlias, joinType, joinCondition, joinHint); } private Join.JoinHint getJoinHint(OpenSearchPPLParser.JoinHintListContext ctx) { @@ -610,4 +615,19 @@ private String getTextInQuery(ParserRuleContext ctx) { Token stop = ctx.getStop(); return query.substring(start.getStartIndex(), stop.getStopIndex() + 1); } + + /** + * Try to wrap the plan with a project node of this AllFields expression. Only wrap it if the plan + * is not a project node or if the project is type of excluded. + * + * @param plan The input plan needs to be wrapped with a project + * @return The wrapped plan of the input plan, i.e., project(plan) + */ + private UnresolvedPlan projectExceptMeta(UnresolvedPlan plan) { + if ((plan instanceof Project) && !((Project) plan).isExcluded()) { + return plan; + } else { + return new Project(ImmutableList.of(AllFieldsExcludeMeta.of())).attach(plan); + } + } } diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java index 52d1208873b..68e1a2517fc 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java @@ -16,8 +16,11 @@ import org.apache.commons.lang3.tuple.ImmutablePair; import org.apache.commons.lang3.tuple.Pair; import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.AggregateFunction; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.AllFields; +import org.opensearch.sql.ast.expression.AllFieldsExcludeMeta; import org.opensearch.sql.ast.expression.And; import org.opensearch.sql.ast.expression.Argument; import org.opensearch.sql.ast.expression.Between; @@ -153,13 +156,15 @@ private String formatFieldAlias(java.util.Map fieldMap) { @Override public String visitSubqueryAlias(SubqueryAlias node, String context) { - String child = node.getChild().get(0).accept(this, context); - if (node.getChild().get(0).getChild().isEmpty()) { - return StringUtils.format("%s as %s", child, node.getAlias()); - } else { - // add "[]" only if its child is not a root - return StringUtils.format("[ %s ] as %s", child, node.getAlias()); + Node childNode = node.getChild().get(0); + String child = childNode.accept(this, context); + if (childNode instanceof Project project + && project.getProjectList().get(0) instanceof AllFields) { + childNode = childNode.getChild().get(0); } + // add "[]" only if its child is not a root + String format = childNode.getChild().isEmpty() ? "%s as %s" : "[ %s ] as %s"; + return StringUtils.format(format, child, node.getAlias()); } @Override @@ -236,6 +241,10 @@ public String visitProject(Project node, String context) { String arg = "+"; String fields = visitExpressionList(node.getProjectList()); + if (Strings.isNullOrEmpty(fields)) { + return child; + } + if (node.hasArgument()) { Argument argument = node.getArgExprList().get(0); Boolean exclude = (Boolean) argument.getValue().getValue(); @@ -458,6 +467,16 @@ public String visitField(Field node, String context) { return node.getField().toString(); } + @Override + public String visitAllFields(AllFields node, String context) { + return ""; + } + + @Override + public String visitAllFieldsExcludeMeta(AllFieldsExcludeMeta node, String context) { + return ""; + } + @Override public String visitAlias(Alias node, String context) { String expr = node.getDelegated().accept(this, context); diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java index 23a4a55da45..bdbc360713c 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java @@ -214,10 +214,10 @@ private UnresolvedExpression visitSelectItem(SelectElementContext ctx) { UnresolvedExpression expr = visitAstExpression(ctx.expression()); if (ctx.alias() == null) { - return new Alias(name, expr); + return Alias.newAliasAllowMetaMetaField(name, expr, null); } else { String alias = StringUtils.unquoteIdentifier(ctx.alias().getText()); - return new Alias(name, expr, alias); + return Alias.newAliasAllowMetaMetaField(name, expr, alias); } } }