Merge remote-tracking branch 'origin/integ-textsearch-improvements' i…

…nto dev-spike-imrove-error-reporting Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Bit-Quill · Sep 8, 2022 · f9757cf · f9757cf
2 parents b599929 + e76dc6e
commit f9757cf
Show file tree

Hide file tree

Showing 10 changed files with 32 additions and 148 deletions.
diff --git a/build.gradle b/build.gradle
@@ -127,11 +127,11 @@ jacocoTestCoverageVerification {
 check.dependsOn jacocoTestCoverageVerification
 
 // TODO: fix code style in main and test source code
-subprojects {
+allprojects {
     apply plugin: 'checkstyle'
     checkstyle {
         configFile rootProject.file("config/checkstyle/google_checks.xml")
-        toolVersion "8.29"
+        toolVersion "10.3.2"
         configProperties = [
                 "org.checkstyle.google.suppressionfilter.config": rootProject.file("config/checkstyle/suppressions.xml")]
         ignoreFailures = false

diff --git a/config/checkstyle/google_checks.xml b/config/checkstyle/google_checks.xml
@@ -279,7 +279,7 @@
                 value="CLASS_DEF, INTERFACE_DEF, ENUM_DEF, METHOD_DEF, CTOR_DEF, VARIABLE_DEF"/>
     </module>
     <module name="JavadocMethod">
-      <property name="scope" value="public"/>
+      <property name="accessModifiers" value="public"/>
       <property name="allowMissingParamTags" value="true"/>
       <property name="allowMissingReturnTag" value="true"/>
       <property name="allowedAnnotations" value="Override, Test"/>

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
@@ -9,13 +9,9 @@
 import static org.opensearch.sql.data.type.ExprCoreType.STRUCT;
 
 import com.google.common.collect.ImmutableMap;
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.stream.Collectors;
 import lombok.experimental.UtilityClass;
-import org.opensearch.sql.ast.dsl.AstDSL;
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
@@ -27,16 +23,6 @@
 
 @UtilityClass
 public class OpenSearchFunctions {
-
-  public static final int MATCH_MAX_NUM_PARAMETERS = 14;
-  public static final int MATCH_BOOL_PREFIX_MAX_NUM_PARAMETERS = 9;
-  public static final int MATCH_PHRASE_MAX_NUM_PARAMETERS = 5;
-  public static final int MIN_NUM_PARAMETERS = 2;
-  public static final int MULTI_MATCH_MAX_NUM_PARAMETERS = 17;
-  public static final int SIMPLE_QUERY_STRING_MAX_NUM_PARAMETERS = 14;
-  public static final int QUERY_STRING_MAX_NUM_PARAMETERS = 25;
-  public static final int MATCH_PHRASE_PREFIX_MAX_NUM_PARAMETERS = 7;
-
   /**
    * Add functions specific to OpenSearch to repository.
    */
@@ -69,52 +55,32 @@ private static FunctionResolver match_bool_prefix() {
 
   private static DefaultFunctionResolver match() {
     FunctionName funcName = BuiltinFunctionName.MATCH.getName();
-    return getRelevanceFunctionResolver(funcName, MATCH_MAX_NUM_PARAMETERS, STRING);
+    return new RelevanceFunctionResolver(funcName, STRING);
   }
 
   private static DefaultFunctionResolver match_phrase_prefix() {
     FunctionName funcName = BuiltinFunctionName.MATCH_PHRASE_PREFIX.getName();
-    return getRelevanceFunctionResolver(funcName, MATCH_PHRASE_PREFIX_MAX_NUM_PARAMETERS, STRING);
+    return new RelevanceFunctionResolver(funcName, STRING);
   }
 
   private static DefaultFunctionResolver match_phrase(BuiltinFunctionName matchPhrase) {
     FunctionName funcName = matchPhrase.getName();
-    return getRelevanceFunctionResolver(funcName, MATCH_PHRASE_MAX_NUM_PARAMETERS, STRING);
+    return new RelevanceFunctionResolver(funcName, STRING);
   }
 
   private static DefaultFunctionResolver multi_match() {
     FunctionName funcName = BuiltinFunctionName.MULTI_MATCH.getName();
-    return getRelevanceFunctionResolver(funcName, MULTI_MATCH_MAX_NUM_PARAMETERS, STRUCT);
+    return new RelevanceFunctionResolver(funcName, STRUCT);
   }
 
   private static DefaultFunctionResolver simple_query_string() {
     FunctionName funcName = BuiltinFunctionName.SIMPLE_QUERY_STRING.getName();
-    return getRelevanceFunctionResolver(funcName, SIMPLE_QUERY_STRING_MAX_NUM_PARAMETERS, STRUCT);
+    return new RelevanceFunctionResolver(funcName, STRUCT);
   }
 
   private static DefaultFunctionResolver query_string() {
     FunctionName funcName = BuiltinFunctionName.QUERY_STRING.getName();
-    return getRelevanceFunctionResolver(funcName, QUERY_STRING_MAX_NUM_PARAMETERS, STRUCT);
-  }
-
-
-  private static DefaultFunctionResolver getRelevanceFunctionResolver(
-      FunctionName funcName, int maxNumParameters, ExprCoreType firstArgType) {
-    return new DefaultFunctionResolver(funcName,
-      getRelevanceFunctionSignatureMap(funcName, maxNumParameters, firstArgType));
-  }
-
-  private static Map<FunctionSignature, FunctionBuilder> getRelevanceFunctionSignatureMap(
-      FunctionName funcName, int maxNumParameters, ExprCoreType firstArgType) {
-    FunctionBuilder buildFunction = args -> new OpenSearchFunction(funcName, args);
-    var signatureMapBuilder = ImmutableMap.<FunctionSignature, FunctionBuilder>builder();
-    for (int numParameters = MIN_NUM_PARAMETERS;
-         numParameters <= maxNumParameters; numParameters++) {
-      List<ExprType> args = new ArrayList<>(Collections.nCopies(numParameters - 1, STRING));
-      args.add(0, firstArgType);
-      signatureMapBuilder.put(new FunctionSignature(funcName, args), buildFunction);
-    }
-    return signatureMapBuilder.build();
+    return new RelevanceFunctionResolver(funcName, STRUCT);
   }
 
   public static class OpenSearchFunction extends FunctionExpression {

diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java
@@ -736,11 +736,11 @@ public void kmeanns_relation() {
   public void ad_batchRCF_relation() {
     Map<String, Literal> argumentMap =
             new HashMap<String, Literal>() {{
-        put("shingle_size", new Literal(8, DataType.INTEGER));
-      }};
+                put("shingle_size", new Literal(8, DataType.INTEGER));
+            }};
     assertAnalyzeEqual(
-            new LogicalAD(LogicalPlanDSL.relation("schema"), argumentMap),
-            new AD(AstDSL.relation("schema"), argumentMap)
+        new LogicalAD(LogicalPlanDSL.relation("schema"), argumentMap),
+        new AD(AstDSL.relation("schema"), argumentMap)
     );
   }
 

diff --git a/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/logical/LogicalPlanNodeVisitorTest.java
@@ -134,8 +134,8 @@ public void testAbstractPlanNodeVisitorShouldReturnNull() {
               put("shingle_size", new Literal(8, DataType.INTEGER));
               put("time_decay", new Literal(0.0001, DataType.DOUBLE));
               put("time_field", new Literal(null, DataType.STRING));
-        }
-      });
+            }
+        });
     assertNull(ad.accept(new LogicalPlanNodeVisitor<Integer, Object>() {
     }, null));
   }

diff --git a/...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java b/...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java
@@ -71,7 +71,7 @@ public ExpressionScript(Expression expression) {
    * Evaluate on the doc generate by the doc provider.
    * @param docProvider doc provider.
    * @param evaluator evaluator
-   * @return
+   * @return expr value
    */
   public ExprValue execute(Supplier<Map<String, ScriptDocValues<?>>> docProvider,
                          BiFunction<Expression,

diff --git a/.../org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQuery.java b/.../org/opensearch/sql/opensearch/storage/script/filter/lucene/relevance/RelevanceQuery.java
@@ -45,7 +45,6 @@ public QueryBuilder build(FunctionExpression func) {
     while (iterator.hasNext()) {
       NamedArgumentExpression arg = (NamedArgumentExpression) iterator.next();
       String argNormalized = arg.getArgName().toLowerCase();
-
       if (visitedParms.contains(argNormalized)) {
         throw new SemanticCheckException(String.format("Parameter '%s' can only be specified once.",
             arg.getArgName()));

diff --git a/...va/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/...va/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java
@@ -263,12 +263,13 @@ public void testVisitMlCommons() {
     MLCommonsOperator mlCommonsOperator =
             new MLCommonsOperator(
               values(emptyList()), "kmeans",
-                    new HashMap<String, Literal>() {{
-          put("centroids", new Literal(3, DataType.INTEGER));
-          put("iterations", new Literal(2, DataType.INTEGER));
-          put("distance_type", new Literal(null, DataType.STRING));
-        }
-      }, nodeClient);
+                new HashMap<String, Literal>() {{
+                  put("centroids", new Literal(3, DataType.INTEGER));
+                  put("iterations", new Literal(2, DataType.INTEGER));
+                  put("distance_type", new Literal(null, DataType.STRING));
+                }},
+                nodeClient
+            );
 
     assertEquals(executionProtector.doProtect(mlCommonsOperator),
             executionProtector.visitMLCommons(mlCommonsOperator, null));
@@ -279,13 +280,14 @@ public void testVisitAD() {
     NodeClient nodeClient = mock(NodeClient.class);
     ADOperator adOperator =
             new ADOperator(
-                    values(emptyList()),
-                    new HashMap<String, Literal>() {{
-          put("shingle_size", new Literal(8, DataType.INTEGER));
-          put("time_decay", new Literal(0.0001, DataType.DOUBLE));
-          put("time_field", new Literal(null, DataType.STRING));
-        }
-      }, nodeClient);
+                values(emptyList()),
+                new HashMap<String, Literal>() {{
+                  put("shingle_size", new Literal(8, DataType.INTEGER));
+                  put("time_decay", new Literal(0.0001, DataType.DOUBLE));
+                  put("time_field", new Literal(null, DataType.STRING));
+                }},
+                nodeClient
+            );
 
     assertEquals(executionProtector.doProtect(adOperator),
             executionProtector.visitAD(adOperator, null));

diff --git a/...test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java b/...test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java
@@ -897,41 +897,6 @@ void relevancy_func_invalid_arg_values() {
     assertTrue(msg.startsWith("Invalid time_zone value: '42'."));
   }
 
-  @Test
-  void match_phrase_missing_field() {
-    var msg = assertThrows(ExpressionEvaluationException.class, () ->
-        dsl.match_phrase(
-            dsl.namedArgument("query", literal("search query")))).getMessage();
-    assertEquals("match_phrase function expected {[STRING,STRING],[STRING,STRING,STRING],"
-          + "[STRING,STRING,STRING,STRING],[STRING,STRING,STRING,STRING,STRING]}, but get [STRING]",
-          msg);
-  }
-
-  @Test
-  void match_phrase_missing_query() {
-    var msg = assertThrows(ExpressionEvaluationException.class, () ->
-        dsl.match_phrase(
-            dsl.namedArgument("field", literal("message")))).getMessage();
-    assertEquals("match_phrase function expected {[STRING,STRING],[STRING,STRING,STRING],"
-          + "[STRING,STRING,STRING,STRING],[STRING,STRING,STRING,STRING,STRING]}, but get [STRING]",
-          msg);
-  }
-
-  @Test
-  void match_phrase_too_many_args() {
-    var msg = assertThrows(ExpressionEvaluationException.class, () ->
-        dsl.match_phrase(
-            dsl.namedArgument("one", literal("1")),
-            dsl.namedArgument("two", literal("2")),
-            dsl.namedArgument("three", literal("3")),
-            dsl.namedArgument("four", literal("4")),
-            dsl.namedArgument("fix", literal("5")),
-            dsl.namedArgument("six", literal("6"))
-                )).getMessage();
-    assertEquals("match_phrase function expected {[STRING,STRING],[STRING,STRING,STRING],"
-          + "[STRING,STRING,STRING,STRING],[STRING,STRING,STRING,STRING,STRING]}, but get "
-          + "[STRING,STRING,STRING,STRING,STRING,STRING]", msg);
-  }
 
 
   @Test
@@ -955,55 +920,6 @@ void should_build_match_bool_prefix_query_with_default_parameters() {
                 dsl.namedArgument("query", literal("search query")))));
   }
 
-  @Test
-  void multi_match_missing_fields() {
-    var msg = assertThrows(ExpressionEvaluationException.class, () ->
-        dsl.multi_match(
-            dsl.namedArgument("query", literal("search query")))).getMessage();
-    assertEquals("multi_match function expected {[STRUCT,STRING],[STRUCT,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING],[STRUCT,STRING,"
-          + "STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING]}, but get [STRING]",
-          msg);
-  }
-
-  @Test
-  void multi_match_missing_query() {
-    var msg = assertThrows(ExpressionEvaluationException.class, () ->
-        dsl.multi_match(
-            dsl.namedArgument("fields", DSL.literal(
-                new ExprTupleValue(new LinkedHashMap<>(ImmutableMap.of(
-                    "field1", ExprValueUtils.floatValue(1.F),
-                    "field2", ExprValueUtils.floatValue(.3F)))))))).getMessage();
-    assertEquals("multi_match function expected {[STRUCT,STRING],[STRUCT,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING],[STRUCT,STRING,"
-          + "STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],"
-          + "[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING],[STRUCT,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING],[STRUCT,STRING,"
-          + "STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,STRING,"
-          + "STRING,STRING,STRING,STRING]}, but get [STRUCT]",
-          msg);
-  }
-
   @Test
   void should_build_match_phrase_prefix_query_with_default_parameters() {
     assertJsonEquals(

diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java
@@ -24,6 +24,7 @@ public enum Format {
   private final String formatName;
 
   private static final Map<String, Format> ALL_FORMATS;
+
   static {
     ImmutableMap.Builder<String, Format> builder = new ImmutableMap.Builder<>();
     for (Format format : Format.values()) {