Code cleanup and adding more tests

* Cleaned up how the fieldTypeTolerance setting is accessed * Added an integration test for when fieldTypeTolerance is disabled Signed-off-by: Norman Jordan <[email protected]>
Bit-Quill · Oct 24, 2024 · d58b6dc · d58b6dc
1 parent bd462c7
commit d58b6dc
Show file tree

Hide file tree

Showing 13 changed files with 117 additions and 48 deletions.
diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ObjectFieldSelectIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ObjectFieldSelectIT.java
@@ -14,6 +14,7 @@
 import org.json.JSONArray;
 import org.json.JSONObject;
 import org.junit.Test;
+import org.opensearch.sql.common.setting.Settings;
 import org.opensearch.sql.legacy.utils.StringUtils;
 
 /**
@@ -82,6 +83,19 @@ public void testSelectObjectFieldOfArrayValuesItself() {
     verifyDataRows(response, rows(new JSONArray("[{\"id\":1},{\"id\":2}]")));
   }
 
+  @Test
+  public void testSelectObjectFieldOfArrayValuesItselfNoFieldTypeTolerance() throws Exception {
+    updateClusterSettings(
+        new ClusterSetting(PERSISTENT, Settings.Key.FIELD_TYPE_TOLERANCE.getKeyValue(), "false"));
+    try {
+      JSONObject response = new JSONObject(query("SELECT accounts FROM %s"));
+      verifyDataRows(response, rows(new JSONObject("{\"id\":1}")));
+    } finally {
+      updateClusterSettings(
+          new ClusterSetting(PERSISTENT, Settings.Key.FIELD_TYPE_TOLERANCE.getKeyValue(), "true"));
+    }
+  }
+
   @Test
   public void testSelectObjectFieldOfArrayValuesInnerFields() {
     JSONObject response = new JSONObject(query("SELECT accounts.id FROM %s"));

diff --git a/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java
@@ -74,6 +74,7 @@ public class OpenSearchExprValueFactory {
   /** The Mapping of Field and ExprType. */
   private final Map<String, OpenSearchDataType> typeMapping;
 
+  /** Whether to support nested value types (such as arrays) */
   private final boolean fieldTypeTolerance;
 
   /**

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java
@@ -126,9 +126,7 @@ public OpenSearchQueryRequest(
    * @param engine OpenSearchSqlEngine to get node-specific context.
    * @throws IOException thrown if reading from input {@code in} fails.
    */
-  public OpenSearchQueryRequest(
-      StreamInput in, OpenSearchStorageEngine engine, boolean fieldTypeTolerance)
-      throws IOException {
+  public OpenSearchQueryRequest(StreamInput in, OpenSearchStorageEngine engine) throws IOException {
     // Deserialize the SearchSourceBuilder from the string representation
     String sourceBuilderString = in.readString();
 
@@ -154,7 +152,8 @@ public OpenSearchQueryRequest(
 
     OpenSearchIndex index = (OpenSearchIndex) engine.getTable(null, indexName.toString());
     exprValueFactory =
-        new OpenSearchExprValueFactory(index.getFieldOpenSearchTypes(), fieldTypeTolerance);
+        new OpenSearchExprValueFactory(
+            index.getFieldOpenSearchTypes(), index.isFieldTypeTolerance());
   }
 
   @Override

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java
@@ -142,7 +142,6 @@ private OpenSearchRequest buildRequestWithScroll(
     int size = requestedTotalSize;
     FetchSourceContext fetchSource = this.sourceBuilder.fetchSource();
     List<String> includes = fetchSource != null ? Arrays.asList(fetchSource.includes()) : List.of();
-    boolean fieldTypeTolerance = settings.getSettingValue(Settings.Key.FIELD_TYPE_TOLERANCE);
 
     if (pageSize == null) {
       if (startFrom + size > maxResultWindow) {

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
@@ -168,7 +168,6 @@ public TableScanBuilder createScanBuilder() {
         requestBuilder ->
             new OpenSearchIndexScan(
                 client,
-                settings.getSettingValue(Settings.Key.FIELD_TYPE_TOLERANCE),
                 requestBuilder.getMaxResponseSize(),
                 requestBuilder.build(indexName, getMaxResultWindow(), cursorKeepAlive, client));
     return new OpenSearchIndexScanBuilder(builder, createScanOperator);

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java
@@ -47,18 +47,12 @@ public class OpenSearchIndexScan extends TableScanOperator implements Serializab
   /** Search response for current batch. */
   private Iterator<ExprValue> iterator;
 
-  private boolean fieldTypeTolerance;
-
   /** Creates index scan based on a provided OpenSearchRequestBuilder. */
   public OpenSearchIndexScan(
-      OpenSearchClient client,
-      boolean fieldTypeTolerance,
-      int maxResponseSize,
-      OpenSearchRequest request) {
+      OpenSearchClient client, int maxResponseSize, OpenSearchRequest request) {
     this.client = client;
     this.maxResponseSize = maxResponseSize;
     this.request = request;
-    this.fieldTypeTolerance = fieldTypeTolerance;
   }
 
   @Override
@@ -133,10 +127,9 @@ public void readExternal(ObjectInput in) throws IOException {
     boolean pointInTimeEnabled =
         Boolean.parseBoolean(
             client.meta().get(Settings.Key.SQL_PAGINATION_API_SEARCH_AFTER.getKeyValue()));
-    fieldTypeTolerance = in.readBoolean();
     try (BytesStreamInput bsi = new BytesStreamInput(requestStream)) {
       if (pointInTimeEnabled) {
-        request = new OpenSearchQueryRequest(bsi, engine, fieldTypeTolerance);
+        request = new OpenSearchQueryRequest(bsi, engine);
       } else {
         request = new OpenSearchScrollRequest(bsi, engine);
       }
@@ -162,7 +155,6 @@ public void writeExternal(ObjectOutput out) throws IOException {
     out.writeInt(reqOut.size());
     out.write(reqAsBytes, 0, reqOut.size());
 
-    out.writeBoolean(fieldTypeTolerance);
     out.writeInt(maxResponseSize);
   }
 }
diff --git a/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java
@@ -853,6 +853,90 @@ public void constructFromOpenSearchArrayReturnAll() {
             .get("structV"));
   }
 
+  /**
+   * Return the all elements if is OpenSearch Array.
+   * https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html.
+   */
+  @Test
+  public void constructFromOpenSearchArrayReturnAllWithArraySupport() {
+    assertEquals(
+        new ExprCollectionValue(List.of(integerValue(1), integerValue(2), integerValue(3))),
+        tupleValue("{\"intV\":[1, 2, 3]}").get("intV"));
+    assertEquals(
+        new ExprCollectionValue(
+            List.of(
+                new ExprTupleValue(
+                    new LinkedHashMap<String, ExprValue>() {
+                      {
+                        put("id", integerValue(1));
+                        put("state", stringValue("WA"));
+                      }
+                    }),
+                new ExprTupleValue(
+                    new LinkedHashMap<String, ExprValue>() {
+                      {
+                        put("id", integerValue(2));
+                        put("state", stringValue("CA"));
+                      }
+                    }))),
+        tupleValueWithArraySupport(
+                "{\"structV\":[{\"id\":1,\"state\":\"WA\"},{\"id\":2,\"state\":\"CA\"}]}}")
+            .get("structV"));
+  }
+
+  /**
+   * Return only the first element if is OpenSearch Array.
+   * https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html.
+   */
+  @Test
+  public void constructFromOpenSearchArrayReturnAllWithoutArraySupport() {
+    assertEquals(
+        new ExprCollectionValue(List.of(integerValue(1), integerValue(2), integerValue(3))),
+        tupleValue("{\"intV\":[1, 2, 3]}").get("intV"));
+    assertEquals(
+        new ExprTupleValue(
+            new LinkedHashMap<String, ExprValue>() {
+              {
+                put("id", integerValue(1));
+                put("state", stringValue("WA"));
+              }
+            }),
+        tupleValueWithoutArraySupport(
+                "{\"structV\":[{\"id\":1,\"state\":\"WA\"},{\"id\":2,\"state\":\"CA\"}]}}")
+            .get("structV"));
+  }
+
+  /**
+   * Return only the first element if is OpenSearch Array.
+   * https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html.
+   */
+  @Test
+  public void constructFromOpenSearchArrayReturnAllWithoutArraySupportNoFieldTolerance() {
+    assertEquals(
+        new ExprCollectionValue(List.of(integerValue(1), integerValue(2), integerValue(3))),
+        tupleValue("{\"intV\":[1, 2, 3]}").get("intV"));
+    assertEquals(
+        new ExprCollectionValue(
+            List.of(
+                new ExprTupleValue(
+                    new LinkedHashMap<String, ExprValue>() {
+                      {
+                        put("id", integerValue(1));
+                        put("state", stringValue("WA"));
+                      }
+                    }),
+                new ExprTupleValue(
+                    new LinkedHashMap<String, ExprValue>() {
+                      {
+                        put("id", integerValue(2));
+                        put("state", stringValue("CA"));
+                      }
+                    }))),
+        tupleValueWithoutArraySupportNoFieldTolerance(
+                "{\"structV\":[{\"id\":1,\"state\":\"WA\"},{\"id\":2,\"state\":\"CA\"}]}}")
+            .get("structV"));
+  }
+
   @Test
   public void constructFromInvalidJsonThrowException() {
     IllegalStateException exception =

diff --git a/...h/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java b/...h/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java
@@ -186,7 +186,6 @@ void explain_successfully() {
     PhysicalPlan plan =
         new OpenSearchIndexScan(
             mock(OpenSearchClient.class),
-            true,
             maxResultWindow,
             requestBuilder.build(
                 name, maxResultWindow, settings.getSettingValue(SQL_CURSOR_KEEP_ALIVE), client));

diff --git a/...va/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/...va/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java
@@ -135,10 +135,7 @@ void test_protect_indexScan() {
                                                 filter(
                                                     resourceMonitor(
                                                         new OpenSearchIndexScan(
-                                                            client,
-                                                            true,
-                                                            maxResultWindow,
-                                                            request)),
+                                                            client, maxResultWindow, request)),
                                                     filterExpr),
                                                 aggregators,
                                                 groupByExprs),
@@ -165,7 +162,7 @@ void test_protect_indexScan() {
                                             PhysicalPlanDSL.agg(
                                                 filter(
                                                     new OpenSearchIndexScan(
-                                                        client, true, maxResultWindow, request),
+                                                        client, maxResultWindow, request),
                                                     filterExpr),
                                                 aggregators,
                                                 groupByExprs),

diff --git a/...earch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequestTest.java b/...earch/src/test/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequestTest.java
@@ -459,7 +459,7 @@ void constructor_serialized() throws IOException {
     when(engine.getTable(null, "sample")).thenReturn(index);
     when(stream.readVInt()).thenReturn(2);
     when(stream.readGenericValue()).thenReturn("sampleSearchAfter");
-    OpenSearchQueryRequest request = new OpenSearchQueryRequest(stream, engine, true);
+    OpenSearchQueryRequest request = new OpenSearchQueryRequest(stream, engine);
     assertNotNull(request);
   }
 

diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexTest.java
@@ -207,10 +207,7 @@ void implementRelationOperatorOnly() {
         new OpenSearchRequestBuilder(QUERY_SIZE_LIMIT, exprValueFactory, settings);
     assertEquals(
         new OpenSearchIndexScan(
-            client,
-            true,
-            200,
-            requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client)),
+            client, 200, requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client)),
         index.implement(index.optimize(plan)));
   }
 
@@ -224,10 +221,7 @@ void implementRelationOperatorWithOptimization() {
         new OpenSearchRequestBuilder(QUERY_SIZE_LIMIT, exprValueFactory, settings);
     assertEquals(
         new OpenSearchIndexScan(
-            client,
-            true,
-            200,
-            requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client)),
+            client, 200, requestBuilder.build(INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client)),
         index.implement(plan));
   }
 
@@ -267,7 +261,6 @@ void implementOtherLogicalOperators() {
                             PhysicalPlanDSL.rename(
                                 new OpenSearchIndexScan(
                                     client,
-                                    true,
                                     QUERY_SIZE_LIMIT,
                                     requestBuilder.build(
                                         INDEX_NAME, maxResultWindow, SCROLL_TIMEOUT, client)),

diff --git a/...st/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java b/...st/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanPaginationTest.java
@@ -78,7 +78,6 @@ void query_empty_result() {
     try (var indexScan =
         new OpenSearchIndexScan(
             client,
-            true,
             MAX_RESULT_WINDOW,
             builder.build(INDEX_NAME, MAX_RESULT_WINDOW, SCROLL_TIMEOUT, client))) {
       indexScan.open();
@@ -107,7 +106,6 @@ void dont_serialize_if_no_cursor() {
     try (var indexScan =
         new OpenSearchIndexScan(
             client,
-            true,
             MAX_RESULT_WINDOW,
             builder.build(INDEX_NAME, MAX_RESULT_WINDOW, SCROLL_TIMEOUT, client))) {
       indexScan.open();