Backport breaking changes to 2.x (#1792)

* Update SQL plugin for core refactor (#1571) Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Fix plugin compilation (#1580) * Changed gradle version and removed values iterator Signed-off-by: Guian Gumpac <guian.gumpac@improving.com> * Update a test to match new indexResponse.aliases() type. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> * Ran ./gradlew wrapper Signed-off-by: Guian Gumpac <guian.gumpac@improving.com> --------- Signed-off-by: Guian Gumpac <guian.gumpac@improving.com> Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> Co-authored-by: MaxKsyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Update sqlite-jdbc to 3.41.2.2 to address CVE-2023-32697 (#1667) * Update sqlite-jdbc to 3.41.2.2 to address CVE-2023-32697 Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> * Don't check column names on H2 results for correctness tests as described in #1667 (comment). Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Address PR review comment. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Signed-off-by: Guian Gumpac <guian.gumpac@improving.com> Co-authored-by: MaxKsyunz <maxk@bitquilltech.com>
opensearch-project · Jun 27, 2023 · e9a009b · e9a009b
1 parent 6532a10
commit e9a009b
Show file tree

Hide file tree

Showing 9 changed files with 69 additions and 68 deletions.
diff --git a/integ-test/build.gradle b/integ-test/build.gradle
@@ -94,7 +94,7 @@ dependencies {
     testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.6.2')
 
     testImplementation group: 'com.h2database', name: 'h2', version: '2.1.214'
-    testImplementation group: 'org.xerial', name: 'sqlite-jdbc', version: '3.28.0'
+    testImplementation group: 'org.xerial', name: 'sqlite-jdbc', version: '3.41.2.2'
     testImplementation group: 'com.google.code.gson', name: 'gson', version: '2.8.9'
 }
 

diff --git a/integ-test/src/test/java/org/opensearch/sql/correctness/runner/resultset/DBResult.java b/integ-test/src/test/java/org/opensearch/sql/correctness/runner/resultset/DBResult.java
@@ -12,8 +12,9 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
-import lombok.EqualsAndHashCode;
+import java.util.stream.Collectors;
 import lombok.Getter;
 import lombok.ToString;
 import org.json.JSONPropertyName;
@@ -24,7 +25,6 @@
  * query with SELECT columns or just *, order of column and row may matter or not. So the internal data structure of this
  * class is passed in from outside either list or set, hash map or linked hash map etc.
  */
-@EqualsAndHashCode(exclude = "databaseName")
 @ToString
 public class DBResult {
 
@@ -191,4 +191,24 @@ private static <T extends Comparable<T>> List<T> sort(Collection<T> collection)
     return list;
   }
 
+  public boolean equals(final Object o) {
+    if (o == this) {
+      return true;
+    }
+    if (!(o instanceof DBResult)) {
+      return false;
+    }
+    final DBResult other = (DBResult) o;
+    // H2 calculates the value before setting column name
+    // for example, for query "select 1 + 1" it returns a column named "2" instead of "1 + 1"
+    boolean skipColumnNameCheck = databaseName.equalsIgnoreCase("h2") || other.databaseName.equalsIgnoreCase("h2");
+    if (!skipColumnNameCheck && !schema.equals(other.schema)) {
+      return false;
+    }
+    if (skipColumnNameCheck && !schema.stream().map(Type::getType).collect(Collectors.toList())
+                    .equals(other.schema.stream().map(Type::getType).collect(Collectors.toList()))) {
+      return false;
+    }
+    return dataRows.equals(other.dataRows);
+  }
 }
diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/esdomain/mapping/IndexMappings.java b/legacy/src/main/java/org/opensearch/sql/legacy/esdomain/mapping/IndexMappings.java
@@ -12,8 +12,6 @@
 import java.util.Objects;
 import org.opensearch.cluster.metadata.MappingMetadata;
 import org.opensearch.cluster.metadata.Metadata;
-import org.opensearch.common.collect.ImmutableOpenMap;
-import org.opensearch.sql.legacy.domain.Field;
 
 /**
  * Index mappings in the cluster.
@@ -51,7 +49,7 @@ public IndexMappings(Metadata metaData) {
                 indexMetaData -> new FieldMappings(indexMetaData.mapping()));
     }
 
-    public IndexMappings(ImmutableOpenMap<String, MappingMetadata> mappings) {
+    public IndexMappings(Map<String, MappingMetadata> mappings) {
         this.indexMappings = buildMappings(mappings, FieldMappings::new);
     }
 

diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/esdomain/mapping/Mappings.java b/legacy/src/main/java/org/opensearch/sql/legacy/esdomain/mapping/Mappings.java
@@ -6,12 +6,10 @@
 
 package org.opensearch.sql.legacy.esdomain.mapping;
 
-import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
-import com.google.common.collect.ImmutableMap;
 import java.util.Collection;
 import java.util.Map;
 import java.util.function.Function;
-import org.opensearch.common.collect.ImmutableOpenMap;
+import java.util.stream.Collectors;
 
 /**
  * Mappings interface to provide default implementation (minimal set of Map methods) for subclass in hierarchy.
@@ -47,13 +45,10 @@ default boolean isEmpty() {
     Map<String, T> data();
 
     /**
-     * Convert OpenSearch ImmutableOpenMap<String, X> to JDK Map<String, Y> by applying function: Y func(X)
+     * Build a map from an existing map by applying provided function to each value.
      */
-    default <X, Y> Map<String, Y> buildMappings(ImmutableOpenMap<String, X> mappings, Function<X, Y> func) {
-        ImmutableMap.Builder<String, Y> builder = ImmutableMap.builder();
-        for (ObjectObjectCursor<String, X> mapping : mappings) {
-            builder.put(mapping.key, func.apply(mapping.value));
-        }
-        return builder.build();
+    default <X, Y> Map<String, Y> buildMappings(Map<String, X> mappings, Function<X, Y> func) {
+        return mappings.entrySet().stream().collect(
+            Collectors.toUnmodifiableMap(Map.Entry::getKey, func.compose(Map.Entry::getValue)));
     }
 }
diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/DescribeResultSet.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/format/DescribeResultSet.java
@@ -6,7 +6,6 @@
 
 package org.opensearch.sql.legacy.executor.format;
 
-import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -15,7 +14,6 @@
 import org.opensearch.action.admin.indices.get.GetIndexResponse;
 import org.opensearch.client.Client;
 import org.opensearch.cluster.metadata.MappingMetadata;
-import org.opensearch.common.collect.ImmutableOpenMap;
 import org.opensearch.sql.legacy.domain.IndexStatement;
 import org.opensearch.sql.legacy.executor.format.DataRows.Row;
 import org.opensearch.sql.legacy.executor.format.Schema.Column;
@@ -79,14 +77,14 @@ private List<Column> loadColumns() {
     private List<Row> loadRows() {
         List<Row> rows = new ArrayList<>();
         GetIndexResponse indexResponse = (GetIndexResponse) queryResult;
-        ImmutableOpenMap<String, MappingMetadata> indexMappings = indexResponse.getMappings();
+        Map<String, MappingMetadata> indexMappings = indexResponse.getMappings();
 
         // Iterate through indices in indexMappings
-        for (ObjectObjectCursor<String, MappingMetadata> indexCursor : indexMappings) {
-            String index = indexCursor.key;
+        for (Entry<String, MappingMetadata> indexCursor : indexMappings.entrySet()) {
+            String index = indexCursor.getKey();
 
             if (matchesPatternIfRegex(index, statement.getIndexPattern())) {
-                rows.addAll(loadIndexData(index, indexCursor.value.getSourceAsMap()));
+                rows.addAll(loadIndexData(index, indexCursor.getValue().getSourceAsMap()));
             }
         }
         return rows;

diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java
@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.sql.SQLFeatureNotSupportedException;
 import java.util.List;
+import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import org.mockito.stubbing.Answer;
@@ -227,9 +228,9 @@ public static ClusterService mockClusterService(String mappings) {
         when(mockService.state()).thenReturn(mockState);
         when(mockState.metadata()).thenReturn(mockMetaData);
         try {
-            ImmutableOpenMap.Builder<String, MappingMetadata> builder = ImmutableOpenMap.builder();
-            builder.put(TestsConstants.TEST_INDEX_BANK, IndexMetadata.fromXContent(createParser(mappings)).mapping());
-            when(mockMetaData.findMappings(any(),  any())).thenReturn(builder.build());
+            when(mockMetaData.findMappings(any(),  any())).thenReturn(
+                Map.of(TestsConstants.TEST_INDEX_BANK, IndexMetadata.fromXContent(
+                    createParser(mappings)).mapping()));
         }
         catch (IOException e) {
             throw new IllegalStateException(e);

diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/MultipleIndexClusterUtils.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/MultipleIndexClusterUtils.java
@@ -14,15 +14,14 @@
 import static org.opensearch.sql.legacy.util.CheckScriptContents.mockIndexNameExpressionResolver;
 import static org.opensearch.sql.legacy.util.CheckScriptContents.mockPluginSettings;
 
-import com.google.common.collect.ImmutableMap;
 import java.io.IOException;
 import java.util.Map;
+import java.util.stream.Collectors;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.cluster.metadata.MappingMetadata;
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.cluster.service.ClusterService;
-import org.opensearch.common.collect.ImmutableOpenMap;
 import org.opensearch.sql.legacy.esdomain.LocalClusterState;
 
 /**
@@ -139,57 +138,54 @@ public class MultipleIndexClusterUtils {
             "}";
 
     public static void mockMultipleIndexEnv() {
-        mockLocalClusterState(new ImmutableMap.Builder<String, ImmutableOpenMap<String, MappingMetadata>>()
-                .put(INDEX_ACCOUNT_1, buildIndexMapping(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING))
-                .put(INDEX_ACCOUNT_2, buildIndexMapping(INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING))
-                .put(INDEX_ACCOUNT_ALL, buildIndexMapping(new ImmutableMap.Builder<String, String>()
-                        .put(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING)
-                        .put(INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING)
-                        .build()))
-                .build());
+        mockLocalClusterState(
+            Map.of(INDEX_ACCOUNT_1, buildIndexMapping(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING),
+                INDEX_ACCOUNT_2, buildIndexMapping(INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING),
+                INDEX_ACCOUNT_ALL, buildIndexMapping(Map.of(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING,
+                    INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING))));
     }
 
-    public static void mockLocalClusterState(Map<String, ImmutableOpenMap<String,MappingMetadata>> indexMapping) {
+    public static void mockLocalClusterState(Map<String, Map<String,MappingMetadata>> indexMapping) {
         LocalClusterState.state().setClusterService(mockClusterService(indexMapping));
         LocalClusterState.state().setResolver(mockIndexNameExpressionResolver());
         LocalClusterState.state().setPluginSettings(mockPluginSettings());
     }
 
 
-    public static ClusterService mockClusterService(Map<String, ImmutableOpenMap<String,MappingMetadata>> indexMapping) {
+    public static ClusterService mockClusterService(Map<String, Map<String,MappingMetadata>>
+                                                        indexMapping) {
         ClusterService mockService = mock(ClusterService.class);
         ClusterState mockState = mock(ClusterState.class);
         Metadata mockMetaData = mock(Metadata.class);
 
         when(mockService.state()).thenReturn(mockState);
         when(mockState.metadata()).thenReturn(mockMetaData);
         try {
-            for (Map.Entry<String, ImmutableOpenMap<String, MappingMetadata>> entry : indexMapping.entrySet()) {
-                when(mockMetaData.findMappings(eq(new String[]{entry.getKey()}), any())).thenReturn(entry.getValue());
+            for (var entry : indexMapping.entrySet()) {
+                when(mockMetaData.findMappings(eq(new String[]{entry.getKey()}), any()))
+                    .thenReturn(entry.getValue());
             }
         } catch (IOException e) {
             throw new IllegalStateException(e);
         }
         return mockService;
     }
 
-    private static ImmutableOpenMap<String, MappingMetadata> buildIndexMapping(Map<String, String> indexMapping) {
-        try {
-            ImmutableOpenMap.Builder<String, MappingMetadata> builder = ImmutableOpenMap.builder();
-            for (Map.Entry<String, String> entry : indexMapping.entrySet()) {
-                builder.put(entry.getKey(), IndexMetadata.fromXContent(createParser(entry.getValue())).mapping());
+    private static Map<String, MappingMetadata> buildIndexMapping(Map<String, String> indexMapping) {
+        return indexMapping.entrySet().stream().collect(Collectors.toUnmodifiableMap(
+            Map.Entry::getKey, e -> {
+            try {
+                return IndexMetadata.fromXContent(createParser(e.getValue())).mapping();
+            } catch (IOException ex) {
+                throw new IllegalStateException(ex);
             }
-            return builder.build();
-        } catch (IOException e) {
-            throw new IllegalStateException(e);
-        }
+        }));
+
     }
 
-    private static ImmutableOpenMap<String, MappingMetadata> buildIndexMapping(String index, String mapping) {
+    private static Map<String, MappingMetadata> buildIndexMapping(String index, String mapping) {
         try {
-            ImmutableOpenMap.Builder<String, MappingMetadata> builder = ImmutableOpenMap.builder();
-            builder.put(index, IndexMetadata.fromXContent(createParser(mapping)).mapping());
-            return builder.build();
+            return Map.of(index, IndexMetadata.fromXContent(createParser(mapping)).mapping());
         } catch (IOException e) {
             throw new IllegalStateException(e);
         }

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java
@@ -9,7 +9,6 @@
 import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Streams;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
@@ -89,9 +88,9 @@ public Map<String, IndexMapping> getIndexMappings(String... indexExpression) {
           .prepareGetMappings(indexExpression)
           .setLocal(true)
           .get();
-      return Streams.stream(mappingsResponse.mappings().iterator())
-          .collect(Collectors.toMap(cursor -> cursor.key,
-              cursor -> new IndexMapping(cursor.value)));
+      return mappingsResponse.mappings().entrySet().stream().collect(Collectors.toUnmodifiableMap(
+              Map.Entry::getKey,
+              cursor -> new IndexMapping(cursor.getValue())));
     } catch (IndexNotFoundException e) {
       // Re-throw directly to be treated as client error finally
       throw e;
@@ -152,7 +151,7 @@ public List<String> indices() {
         .setLocal(true)
         .get();
     final Stream<String> aliasStream =
-        ImmutableList.copyOf(indexResponse.aliases().valuesIt()).stream()
+        ImmutableList.copyOf(indexResponse.aliases().values()).stream()
             .flatMap(Collection::stream).map(AliasMetadata::alias);
 
     return Stream.concat(Arrays.stream(indexResponse.getIndices()), aliasStream)

diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java
@@ -399,9 +399,7 @@ void cleanup_rethrows_exception() {
   @Test
   void get_indices() {
     AliasMetadata aliasMetadata = mock(AliasMetadata.class);
-    ImmutableOpenMap.Builder<String, List<AliasMetadata>> builder = ImmutableOpenMap.builder();
-    builder.fPut("index", Arrays.asList(aliasMetadata));
-    final ImmutableOpenMap<String, List<AliasMetadata>> openMap = builder.build();
+    final var openMap = Map.of("index", List.of(aliasMetadata));
     when(aliasMetadata.alias()).thenReturn("index_alias");
     when(nodeClient.admin().indices()
         .prepareGetIndex()
@@ -437,16 +435,12 @@ public void mockNodeClientIndicesMappings(String indexName, String mappings) {
         .setLocal(anyBoolean())
         .get()).thenReturn(mockResponse);
     try {
-      ImmutableOpenMap<String, MappingMetadata> metadata;
+      Map<String, MappingMetadata> metadata;
       if (mappings.isEmpty()) {
-        when(emptyMapping.getSourceAsMap()).thenReturn(ImmutableMap.of());
-        metadata =
-            new ImmutableOpenMap.Builder<String, MappingMetadata>()
-                .fPut(indexName, emptyMapping)
-                .build();
+        when(emptyMapping.getSourceAsMap()).thenReturn(Map.of());
+        metadata = Map.of(indexName, emptyMapping);
       } else {
-        metadata = new ImmutableOpenMap.Builder<String, MappingMetadata>().fPut(indexName,
-            IndexMetadata.fromXContent(createParser(mappings)).mapping()).build();
+        metadata = Map.of(indexName, IndexMetadata.fromXContent(createParser(mappings)).mapping());
       }
       when(mockResponse.mappings()).thenReturn(metadata);
     } catch (IOException e) {