Move fields validation to FunctionExpression interface and derived cl…

…ass OpenSearchFunctions. Signed-off-by: forestmvey <forestv@bitquilltech.com>
opensearch-project · Nov 21, 2022 · a79dfcd · a79dfcd
1 parent a26e587
commit a79dfcd
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 34 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
@@ -68,7 +68,6 @@
 import org.opensearch.sql.exception.SemanticCheckException;
 import org.opensearch.sql.expression.DSL;
 import org.opensearch.sql.expression.Expression;
-import org.opensearch.sql.expression.FunctionExpression;
 import org.opensearch.sql.expression.LiteralExpression;
 import org.opensearch.sql.expression.NamedExpression;
 import org.opensearch.sql.expression.ReferenceExpression;
@@ -222,7 +221,9 @@ public LogicalPlan visitFilter(Filter node, AnalysisContext context) {
     LogicalPlan child = node.getChild().get(0).accept(this, context);
     Expression condition = expressionAnalyzer.analyze(node.getCondition(), context);
 
-    OpenSearchFunctions.validateFieldList((FunctionExpression)condition, context);
+    if (condition instanceof OpenSearchFunctions.OpenSearchFunction) {
+      ((OpenSearchFunctions.OpenSearchFunction)condition).validateParameters(context);
+    }
 
     ExpressionReferenceOptimizer optimizer =
         new ExpressionReferenceOptimizer(expressionAnalyzer.getRepository(), child);

diff --git a/core/src/main/java/org/opensearch/sql/expression/FunctionExpression.java b/core/src/main/java/org/opensearch/sql/expression/FunctionExpression.java
@@ -11,6 +11,7 @@
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
 import lombok.ToString;
+import org.opensearch.sql.analysis.AnalysisContext;
 import org.opensearch.sql.expression.function.FunctionImplementation;
 import org.opensearch.sql.expression.function.FunctionName;
 
@@ -32,4 +33,11 @@ public <T, C> T accept(ExpressionNodeVisitor<T, C> visitor, C context) {
     return visitor.visitFunction(this, context);
   }
 
+  /**
+   * Verify if function queries fields available in type environment.
+   * @param context : Context of fields querying.
+   */
+  public void validateParameters(AnalysisContext context) {
+    return;
+  }
 }
diff --git a/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java b/core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java
@@ -8,7 +8,6 @@
 import static org.opensearch.sql.data.type.ExprCoreType.STRING;
 import static org.opensearch.sql.data.type.ExprCoreType.STRUCT;
 
-import com.google.common.collect.ImmutableList;
 import java.util.List;
 import java.util.stream.Collectors;
 import lombok.experimental.UtilityClass;
@@ -27,14 +26,14 @@
 
 @UtilityClass
 public class OpenSearchFunctions {
-  private final List<String> singleFieldFunctionNames = ImmutableList.of(
+  private final List<String> singleFieldFunctionNames = List.of(
       BuiltinFunctionName.MATCH.name(),
       BuiltinFunctionName.MATCH_BOOL_PREFIX.name(),
       BuiltinFunctionName.MATCHPHRASE.name(),
       BuiltinFunctionName.MATCH_PHRASE_PREFIX.name()
   );
 
-  private final List<String> multiFieldFunctionNames = ImmutableList.of(
+  private final List<String> multiFieldFunctionNames = List.of(
       BuiltinFunctionName.MULTI_MATCH.name(),
       BuiltinFunctionName.SIMPLE_QUERY_STRING.name(),
       BuiltinFunctionName.QUERY_STRING.name()
@@ -58,35 +57,6 @@ public static boolean isMultiFieldFunction(String funcName) {
     return multiFieldFunctionNames.contains(funcName.toUpperCase());
   }
 
-  /**
-   * Verify if function queries fields available in type environment.
-   * @param node : Function used in query.
-   * @param context : Context of fields querying.
-   */
-  public static void validateFieldList(FunctionExpression node, AnalysisContext context) {
-    String funcName = node.getFunctionName().toString();
-
-    TypeEnvironment typeEnv = context.peek();
-    if (isSingleFieldFunction(funcName)) {
-      node.getArguments().stream().map(NamedArgumentExpression.class::cast).filter(arg ->
-          ((arg.getArgName().equals("field")
-              && !arg.getValue().toString().contains("*"))
-          )).findFirst().ifPresent(arg ->
-          typeEnv.resolve(new Symbol(Namespace.FIELD_NAME,
-              StringUtils.unquoteText(arg.getValue().toString()))
-          )
-      );
-    } else if (isMultiFieldFunction(funcName)) {
-      node.getArguments().stream().map(NamedArgumentExpression.class::cast).filter(arg ->
-          arg.getArgName().equals("fields")
-      ).findFirst().ifPresent(fields ->
-          fields.getValue().valueOf(null).tupleValue()
-              .entrySet().stream().filter(k -> !(k.getKey().contains("*"))
-              ).forEach(key -> typeEnv.resolve(new Symbol(Namespace.FIELD_NAME, key.getKey())))
-      );
-    }
-  }
-
   /**
    * Add functions specific to OpenSearch to repository.
    */
@@ -179,5 +149,35 @@ public String toString() {
           .collect(Collectors.toList());
       return String.format("%s(%s)", functionName, String.join(", ", args));
     }
+
+    /**
+     * Verify if function queries fields available in type environment.
+     * @param context : Context of fields querying.
+     */
+    @Override
+    public void validateParameters(AnalysisContext context) {
+      String funcName = this.getFunctionName().toString();
+
+      TypeEnvironment typeEnv = context.peek();
+      if (isSingleFieldFunction(funcName)) {
+        this.getArguments().stream().map(NamedArgumentExpression.class::cast).filter(arg ->
+            ((arg.getArgName().equals("field")
+                && !arg.getValue().toString().contains("*"))
+            )).findFirst().ifPresent(arg ->
+            typeEnv.resolve(new Symbol(Namespace.FIELD_NAME,
+                StringUtils.unquoteText(arg.getValue().toString()))
+            )
+        );
+      } else if (isMultiFieldFunction(funcName)) {
+        this.getArguments().stream().map(NamedArgumentExpression.class::cast).filter(arg ->
+            arg.getArgName().equals("fields")
+        ).findFirst().ifPresent(fields ->
+            fields.getValue().valueOf(null).tupleValue()
+                .entrySet().stream().filter(k -> !(k.getKey().contains("*"))
+                ).forEach(key -> typeEnv.resolve(new Symbol(Namespace.FIELD_NAME, key.getKey())))
+        );
+      }
+    }
+
   }
 }
diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java
@@ -80,11 +80,17 @@
 import org.opensearch.sql.ast.tree.ML;
 import org.opensearch.sql.ast.tree.RareTopN.CommandType;
 import org.opensearch.sql.data.model.ExprTupleValue;
+import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.data.model.ExprValueUtils;
+import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
 import org.opensearch.sql.exception.SemanticCheckException;
 import org.opensearch.sql.expression.DSL;
+import org.opensearch.sql.expression.Expression;
+import org.opensearch.sql.expression.FunctionExpression;
 import org.opensearch.sql.expression.HighlightExpression;
+import org.opensearch.sql.expression.env.Environment;
+import org.opensearch.sql.expression.function.FunctionName;
 import org.opensearch.sql.expression.window.WindowDefinition;
 import org.opensearch.sql.planner.logical.LogicalAD;
 import org.opensearch.sql.planner.logical.LogicalMLCommons;
@@ -269,6 +275,23 @@ public void analyze_filter_aggregation_relation() {
                 aggregate("MIN", qualifiedName("integer_value")), intLiteral(10))));
   }
 
+  @Test
+  public void test_base_class_validate_parameters_method_does_nothing() {
+    var funcExpr = new FunctionExpression(FunctionName.of("func_name"),
+        List.of()) {
+      @Override
+      public ExprValue valueOf(Environment<Expression, ExprValue> valueEnv) {
+        return null;
+      }
+
+      @Override
+      public ExprType type() {
+        return null;
+      }
+    };
+    funcExpr.validateParameters(analysisContext);
+  }
+
   @Test
   public void single_field_relevance_query_semantic_exception() {
     SemanticCheckException exception =
@@ -352,6 +375,19 @@ public void multi_field_relevance_query_mixed_fields_semantic_exception() {
         exception.getMessage());
   }
 
+  @Test
+  public void no_field_relevance_query_semantic_exception() {
+    assertAnalyzeEqual(
+        LogicalPlanDSL.filter(
+            LogicalPlanDSL.relation("schema", table),
+            DSL.query(
+                DSL.namedArgument("query", DSL.literal("string_value:query_value")))),
+        AstDSL.filter(
+            AstDSL.relation("schema"),
+            AstDSL.function("query",
+                AstDSL.unresolvedArg("query", stringLiteral("string_value:query_value")))));
+  }
+
   @Test
   public void multi_field_relevance_query() {
     assertAnalyzeEqual(