Enforce 100% branch coverage for sql and es module

elasticsearch/build.gradle

@@ -54,9 +54,13 @@ jacocoTestCoverageVerification {
                     'com.amazon.opendistroforelasticsearch.sql.elasticsearch.security.SecurityAccess'
             ]
             limit {
+                counter = 'LINE'
+                minimum = 1.0
+            }
+            limit {
+                counter = 'BRANCH'
                 minimum = 1.0
             }
-
         }
     }
     afterEvaluate {

elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java

@@ -142,8 +142,7 @@ public void close() {
   }
 
   private boolean isBoolFilterQuery(QueryBuilder current) {
-    return (current instanceof BoolQueryBuilder)
-        && !((BoolQueryBuilder) current).filter().isEmpty();
+    return (current instanceof BoolQueryBuilder);
   }
 
 }

elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java

@@ -137,7 +137,7 @@ private Object getDocValue(ReferenceExpression field,
     String fieldName = getDocValueName(field);
     ScriptDocValues<?> docValue = docProvider.get().get(fieldName);
     if (docValue == null || docValue.isEmpty()) {
-      return null;
+      return null; // No way to differentiate null and missing from doc value
     }
 
     Object value = docValue.get(0);

elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java

@@ -53,11 +53,10 @@ public boolean execute() {
     return (Boolean) expressionScript.execute(this::getDoc, this::evaluateExpression);
   }
 
-
   private ExprValue evaluateExpression(Expression expression,
                                        Environment<Expression, ExprValue> valueEnv) {
     ExprValue result = expression.valueOf(valueEnv);
-    if (result.isNull() || result.isMissing()) {
+    if (result.isNull()) {
       return ExprBooleanValue.of(false);
     }
 

elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/ElasticsearchResponseTest.java

@@ -16,6 +16,7 @@
 
 package com.amazon.opendistroforelasticsearch.sql.elasticsearch.response;
 
+import static java.util.Collections.emptyList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -75,17 +76,20 @@ void isEmpty() {
                 new TotalHits(2L, TotalHits.Relation.EQUAL_TO),
                 1.0F));
 
-    ElasticsearchResponse response1 = new ElasticsearchResponse(esResponse, factory);
-    assertFalse(response1.isEmpty());
+    assertFalse(new ElasticsearchResponse(esResponse, factory).isEmpty());
 
     when(esResponse.getHits()).thenReturn(SearchHits.empty());
-    ElasticsearchResponse response2 = new ElasticsearchResponse(esResponse, factory);
-    assertTrue(response2.isEmpty());
+    when(esResponse.getAggregations()).thenReturn(null);
+    assertTrue(new ElasticsearchResponse(esResponse, factory).isEmpty());
 
     when(esResponse.getHits())
         .thenReturn(new SearchHits(null, new TotalHits(0, TotalHits.Relation.EQUAL_TO), 0));
     ElasticsearchResponse response3 = new ElasticsearchResponse(esResponse, factory);
     assertTrue(response3.isEmpty());
+
+    when(esResponse.getHits()).thenReturn(SearchHits.empty());
+    when(esResponse.getAggregations()).thenReturn(new Aggregations(emptyList()));
+    assertFalse(new ElasticsearchResponse(esResponse, factory).isEmpty());
   }
 
   @Test

elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScriptTest.java

@@ -23,7 +23,9 @@
 import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD;
 import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal;
 import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref;
+import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
+import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
@@ -38,6 +40,7 @@
 import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig;
 import com.google.common.collect.ImmutableMap;
 import java.time.ZonedDateTime;
+import java.util.List;
 import java.util.Map;
 import lombok.RequiredArgsConstructor;
 import org.apache.lucene.index.LeafReaderContext;
@@ -132,6 +135,14 @@ void can_execute_expression_with_missing_field() {
         .shouldNotMatch();
   }
 
+  @Test
+  void can_execute_expression_with_empty_doc_value() {
+    assertThat()
+        .docValues("name", emptyList())
+        .filterBy(ref("name", STRING))
+        .shouldNotMatch();
+  }
+
   @Test
   void cannot_execute_non_predicate_expression() {
     assertThrow(IllegalStateException.class,
@@ -210,9 +221,13 @@ private LeafDocLookup mockLeafDocLookup(Map<String, ScriptDocValues<?>> docValue
     }
   }
 
-  @RequiredArgsConstructor
   private static class FakeScriptDocValues<T> extends ScriptDocValues<T> {
-    private final T value;
+    private final List<T> values;
+
+    @SuppressWarnings("unchecked")
+    public FakeScriptDocValues(T value) {
+      this.values = (value instanceof List) ? (List<T>) value : singletonList(value);
+    }
 
     @Override
     public void setNextDocId(int docId) {
@@ -221,12 +236,12 @@ public void setNextDocId(int docId) {
 
     @Override
     public T get(int index) {
-      return value;
+      return values.get(index);
     }
 
     @Override
     public int size() {
-      return 1;
+      return values.size();
     }
   }
 

elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilderTest.java

@@ -21,19 +21,19 @@
 import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD;
 import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal;
 import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.when;
 
+import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils;
 import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.serialization.ExpressionSerializer;
 import com.amazon.opendistroforelasticsearch.sql.expression.DSL;
 import com.amazon.opendistroforelasticsearch.sql.expression.Expression;
 import com.amazon.opendistroforelasticsearch.sql.expression.FunctionExpression;
 import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig;
 import com.google.common.collect.ImmutableMap;
 import java.util.Map;
+import org.json.JSONObject;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayNameGeneration;
 import org.junit.jupiter.api.DisplayNameGenerator;
@@ -60,7 +60,7 @@ void set_up() {
 
   @Test
   void should_build_term_query_for_equality_expression() {
-    assertEquals(
+    assertJsonEquals(
         "{\n"
             + "  \"term\" : {\n"
             + "    \"name\" : {\n"
@@ -84,7 +84,7 @@ void should_build_range_query_for_comparison_expression() {
         dsl.gte(params), new Object[]{30, null, true, true});
 
     ranges.forEach((expr, range) ->
-        assertEquals(
+        assertJsonEquals(
             "{\n"
                 + "  \"range\" : {\n"
                 + "    \"age\" : {\n"
@@ -101,7 +101,7 @@ void should_build_range_query_for_comparison_expression() {
 
   @Test
   void should_build_wildcard_query_for_like_expression() {
-    assertEquals(
+    assertJsonEquals(
         "{\n"
             + "  \"wildcard\" : {\n"
             + "    \"name\" : {\n"
@@ -116,13 +116,26 @@ void should_build_wildcard_query_for_like_expression() {
   }
 
   @Test
-  void should_build_script_query_for_function_expression() {
-    doAnswer(invocation -> {
-      Expression expr = invocation.getArgument(0);
-      return expr.toString();
-    }).when(serializer).serialize(any());
+  void should_build_script_query_for_unsupported_lucene_query() {
+    mockToStringSerializer();
+    assertJsonEquals(
+        "{\n"
+            + "  \"script\" : {\n"
+            + "    \"script\" : {\n"
+            + "      \"source\" : \"is not null(age)\",\n"
+            + "      \"lang\" : \"opendistro_expression\"\n"
+            + "    },\n"
+            + "    \"boost\" : 1.0\n"
+            + "  }\n"
+            + "}",
+        buildQuery(
+            dsl.isnotnull(ref("age", INTEGER))));
+  }
 
-    assertEquals(
+  @Test
+  void should_build_script_query_for_function_expression() {
+    mockToStringSerializer();
+    assertJsonEquals(
         "{\n"
             + "  \"script\" : {\n"
             + "    \"script\" : {\n"
@@ -139,12 +152,8 @@ void should_build_script_query_for_function_expression() {
 
   @Test
   void should_build_script_query_for_comparison_between_fields() {
-    doAnswer(invocation -> {
-      Expression expr = invocation.getArgument(0);
-      return expr.toString();
-    }).when(serializer).serialize(any());
-
-    assertEquals(
+    mockToStringSerializer();
+    assertJsonEquals(
         "{\n"
             + "  \"script\" : {\n"
             + "    \"script\" : {\n"
@@ -170,7 +179,7 @@ void should_build_bool_query_for_and_or_expression() {
     };
 
     for (int i = 0; i < names.length; i++) {
-      assertEquals(
+      assertJsonEquals(
           "{\n"
               + "  \"bool\" : {\n"
               + "    \"" + names[i] + "\" : [\n"
@@ -201,7 +210,7 @@ void should_build_bool_query_for_and_or_expression() {
 
   @Test
   void should_build_bool_query_for_not_expression() {
-    assertEquals(
+    assertJsonEquals(
         "{\n"
             + "  \"bool\" : {\n"
             + "    \"must_not\" : [\n"
@@ -226,7 +235,7 @@ void should_build_bool_query_for_not_expression() {
 
   @Test
   void should_use_keyword_for_multi_field_in_equality_expression() {
-    assertEquals(
+    assertJsonEquals(
         "{\n"
             + "  \"term\" : {\n"
             + "    \"name.keyword\" : {\n"
@@ -242,7 +251,7 @@ void should_use_keyword_for_multi_field_in_equality_expression() {
 
   @Test
   void should_use_keyword_for_multi_field_in_like_expression() {
-    assertEquals(
+    assertJsonEquals(
         "{\n"
             + "  \"wildcard\" : {\n"
             + "    \"name.keyword\" : {\n"
@@ -256,8 +265,20 @@ void should_use_keyword_for_multi_field_in_like_expression() {
                 ref("name", ES_TEXT_KEYWORD), literal("John%"))));
   }
 
+  private static void assertJsonEquals(String expected, String actual) {
+    assertTrue(new JSONObject(expected).similar(new JSONObject(actual)),
+        StringUtils.format("Expected: %s, actual: %s", expected, actual));
+  }
+
   private String buildQuery(Expression expr) {
     return filterQueryBuilder.build(expr).toString();
   }
 
+  private void mockToStringSerializer() {
+    doAnswer(invocation -> {
+      Expression expr = invocation.getArgument(0);
+      return expr.toString();
+    }).when(serializer).serialize(any());
+  }
+
 }

elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/lucene/LuceneQueryTest.java

@@ -16,15 +16,25 @@
 
 package com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.filter.lucene;
 
+import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.amazon.opendistroforelasticsearch.sql.expression.DSL;
+import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig;
 import org.junit.jupiter.api.DisplayNameGeneration;
 import org.junit.jupiter.api.DisplayNameGenerator;
 import org.junit.jupiter.api.Test;
 
 @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
 class LuceneQueryTest {
 
+  @Test
+  void should_not_support_single_argument_by_default() {
+    DSL dsl = new ExpressionConfig().dsl(new ExpressionConfig().functionRepository());
+    assertFalse(new LuceneQuery(){}.canSupport(dsl.abs(DSL.ref("age", INTEGER))));
+  }
+
   @Test
   void should_throw_exception_if_not_implemented() {
     assertThrows(UnsupportedOperationException.class, () ->

sql/build.gradle

@@ -67,9 +67,13 @@ jacocoTestCoverageVerification {
     violationRules {
         rule {
             limit {
+                counter = 'LINE'
+                minimum = 1.0
+            }
+            limit {
+                counter = 'BRANCH'
                 minimum = 1.0
             }
-
         }
     }
     afterEvaluate {

sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequest.java

@@ -17,6 +17,8 @@
 package com.amazon.opendistroforelasticsearch.sql.sql.domain;
 
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
+import java.util.Set;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
@@ -32,6 +34,9 @@
 @RequiredArgsConstructor
 public class SQLQueryRequest {
 
+  private static final Set<String> QUERY_FIELD = ImmutableSet.of("query");
+  private static final Set<String> QUERY_AND_FETCH_SIZE = ImmutableSet.of("query", "fetch_size");
+
   /**
    * JSON payload in REST request.
    */
@@ -54,15 +59,15 @@ public class SQLQueryRequest {
 
   /**
    * Pre-check if the request can be supported by meeting the following criteria:
-   *  1.Only "query" field in payload. In other word, it's not a cursor request
-   *   (with either non-zero "fetch_size" or "cursor" field) or request with extra field
-   *   such as "filter".
+   *  1.Only "query" field or "query" and "fetch_size=0" in payload. In other word,
+   *  it's not a cursor request with either non-zero "fetch_size" or "cursor" field,
+   *  or request with extra field such as "filter".
    *  2.Response format expected is default JDBC format.
    *
    * @return  true if supported.
    */
   public boolean isSupported() {
-    return isOnlyQueryFieldInPayload()
+    return (isOnlyQueryFieldInPayload() || isOnlyQueryAndFetchSizeZeroInPayload())
         && isDefaultFormat();
   }
 
@@ -75,9 +80,12 @@ public boolean isExplainRequest() {
   }
 
   private boolean isOnlyQueryFieldInPayload() {
-    return (jsonContent.keySet().size() == 1 && jsonContent.has("query"))
-        || (jsonContent.keySet().size() == 2 && jsonContent.has("query")
-            && jsonContent.has("fetch_size") && jsonContent.getInt("fetch_size") == 0);
+    return QUERY_FIELD.equals(jsonContent.keySet());
+  }
+
+  private boolean isOnlyQueryAndFetchSizeZeroInPayload() {
+    return QUERY_AND_FETCH_SIZE.equals(jsonContent.keySet())
+        && (jsonContent.getInt("fetch_size") == 0);
   }
 
   private boolean isDefaultFormat() {