From e9a009bfb6cee7f98689df628ebebac0a882b1ea Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Tue, 27 Jun 2023 14:13:10 -0700 Subject: [PATCH] Backport breaking changes to 2.x (#1792) * Update SQL plugin for core refactor (#1571) Signed-off-by: MaxKsyunz Signed-off-by: Yury-Fridlyand * Fix plugin compilation (#1580) * Changed gradle version and removed values iterator Signed-off-by: Guian Gumpac * Update a test to match new indexResponse.aliases() type. Signed-off-by: MaxKsyunz * Ran ./gradlew wrapper Signed-off-by: Guian Gumpac --------- Signed-off-by: Guian Gumpac Signed-off-by: MaxKsyunz Co-authored-by: MaxKsyunz Signed-off-by: Yury-Fridlyand * 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 * Don't check column names on H2 results for correctness tests as described in https://github.com/opensearch-project/sql/pull/1667#issuecomment-1603659136. Signed-off-by: Yury-Fridlyand * Address PR review comment. Signed-off-by: Yury-Fridlyand --------- Signed-off-by: MaxKsyunz Signed-off-by: Yury-Fridlyand Co-authored-by: Yury-Fridlyand Signed-off-by: Yury-Fridlyand --------- Signed-off-by: MaxKsyunz Signed-off-by: Yury-Fridlyand Signed-off-by: Guian Gumpac Co-authored-by: MaxKsyunz --- integ-test/build.gradle | 2 +- .../runner/resultset/DBResult.java | 24 ++++++++- .../esdomain/mapping/IndexMappings.java | 4 +- .../sql/legacy/esdomain/mapping/Mappings.java | 15 ++---- .../executor/format/DescribeResultSet.java | 10 ++-- .../sql/legacy/util/CheckScriptContents.java | 7 +-- .../util/MultipleIndexClusterUtils.java | 50 +++++++++---------- .../client/OpenSearchNodeClient.java | 9 ++-- .../client/OpenSearchNodeClientTest.java | 16 ++---- 9 files changed, 69 insertions(+), 68 deletions(-) diff --git a/integ-test/build.gradle b/integ-test/build.gradle index 11c26722ae..b7bce81b02 100644 --- 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 index 0899a6e2c4..eb522b008d 100644 --- 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 > List sort(Collection 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 index a1b54faddf..3b89eef02f 100644 --- 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 mappings) { + public IndexMappings(Map 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 index 23e9eefd06..03bfcaf030 100644 --- 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 data(); /** - * Convert OpenSearch ImmutableOpenMap to JDK Map by applying function: Y func(X) + * Build a map from an existing map by applying provided function to each value. */ - default Map buildMappings(ImmutableOpenMap mappings, Function func) { - ImmutableMap.Builder builder = ImmutableMap.builder(); - for (ObjectObjectCursor mapping : mappings) { - builder.put(mapping.key, func.apply(mapping.value)); - } - return builder.build(); + default Map buildMappings(Map mappings, Function 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 index 7d09aa5996..0cccf73268 100644 --- 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 loadColumns() { private List loadRows() { List rows = new ArrayList<>(); GetIndexResponse indexResponse = (GetIndexResponse) queryResult; - ImmutableOpenMap indexMappings = indexResponse.getMappings(); + Map indexMappings = indexResponse.getMappings(); // Iterate through indices in indexMappings - for (ObjectObjectCursor indexCursor : indexMappings) { - String index = indexCursor.key; + for (Entry 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 index d4b8a88707..ff93117f4a 100644 --- 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 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 index 0213cac01b..ff15cd698c 100644 --- 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,24 +138,22 @@ public class MultipleIndexClusterUtils { "}"; public static void mockMultipleIndexEnv() { - mockLocalClusterState(new ImmutableMap.Builder>() - .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() - .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> indexMapping) { + public static void mockLocalClusterState(Map> indexMapping) { LocalClusterState.state().setClusterService(mockClusterService(indexMapping)); LocalClusterState.state().setResolver(mockIndexNameExpressionResolver()); LocalClusterState.state().setPluginSettings(mockPluginSettings()); } - public static ClusterService mockClusterService(Map> indexMapping) { + public static ClusterService mockClusterService(Map> + indexMapping) { ClusterService mockService = mock(ClusterService.class); ClusterState mockState = mock(ClusterState.class); Metadata mockMetaData = mock(Metadata.class); @@ -164,8 +161,9 @@ public static ClusterService mockClusterService(Map> 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); @@ -173,23 +171,21 @@ public static ClusterService mockClusterService(Map buildIndexMapping(Map indexMapping) { - try { - ImmutableOpenMap.Builder builder = ImmutableOpenMap.builder(); - for (Map.Entry entry : indexMapping.entrySet()) { - builder.put(entry.getKey(), IndexMetadata.fromXContent(createParser(entry.getValue())).mapping()); + private static Map buildIndexMapping(Map 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 buildIndexMapping(String index, String mapping) { + private static Map buildIndexMapping(String index, String mapping) { try { - ImmutableOpenMap.Builder 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 index e4f25dabbd..79afd8829b 100644 --- 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 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 indices() { .setLocal(true) .get(); final Stream 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 index f490c67b34..2de0214baa 100644 --- 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> builder = ImmutableOpenMap.builder(); - builder.fPut("index", Arrays.asList(aliasMetadata)); - final ImmutableOpenMap> 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 metadata; + Map metadata; if (mappings.isEmpty()) { - when(emptyMapping.getSourceAsMap()).thenReturn(ImmutableMap.of()); - metadata = - new ImmutableOpenMap.Builder() - .fPut(indexName, emptyMapping) - .build(); + when(emptyMapping.getSourceAsMap()).thenReturn(Map.of()); + metadata = Map.of(indexName, emptyMapping); } else { - metadata = new ImmutableOpenMap.Builder().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) {