From b028fe9d5581c26b93f1265fa95314cf17921eb6 Mon Sep 17 00:00:00 2001 From: egalpin Date: Wed, 28 Feb 2024 09:54:35 -0800 Subject: [PATCH 1/4] Prefers apache.commons StringUtils.split over String#split --- .../pinot/common/utils/config/QueryOptionsUtils.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java index 881aa30b8a1a..bb5f5b3f99a4 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java @@ -27,6 +27,7 @@ import java.util.Map; import java.util.Set; import javax.annotation.Nullable; +import org.apache.commons.lang3.StringUtils; import org.apache.pinot.spi.config.table.FieldConfig; import org.apache.pinot.spi.utils.CommonConstants; import org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey; @@ -157,16 +158,16 @@ public static Map> getIndexSkipConfig(Map> indexSkipConfig = new HashMap<>(); for (String columnConf : perColumnIndexSkip) { - String[] conf = columnConf.split("="); + String[] conf = StringUtils.split(columnConf, '='); if (conf.length != 2) { throw new RuntimeException("Invalid format for " + QueryOptionKey.INDEX_SKIP_CONFIG + ". Example of valid format: SET indexSkipConfig='col1=inverted,range&col2=inverted'"); } String columnName = conf[0]; - String[] indexTypes = conf[1].split(","); + String[] indexTypes = StringUtils.split(conf[1], ','); for (String indexType : indexTypes) { indexSkipConfig.computeIfAbsent(columnName, k -> new HashSet<>()) From 0a2dff7cee8986bccce33351147c60efc8510ad1 Mon Sep 17 00:00:00 2001 From: egalpin Date: Wed, 28 Feb 2024 11:43:01 -0800 Subject: [PATCH 2/4] Renames indexSkipConfig option to skipIndexes --- .../utils/config/QueryOptionsUtils.java | 20 ++++++------- .../utils/config/QueryOptionsUtilsTest.java | 22 +++++++------- .../plan/maker/InstancePlanMakerImplV2.java | 2 +- .../query/request/context/QueryContext.java | 10 +++---- .../tests/OfflineClusterIntegrationTest.java | 30 +++++++++---------- .../pinot/spi/utils/CommonConstants.java | 2 +- 6 files changed, 42 insertions(+), 44 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java index bb5f5b3f99a4..2356d66e23d6 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java @@ -151,31 +151,31 @@ public static boolean isSkipScanFilterReorder(Map queryOptions) } @Nullable - public static Map> getIndexSkipConfig(Map queryOptions) { - // Example config: indexSkipConfig='col1=inverted,range&col2=inverted' - String indexSkipConfigStr = queryOptions.get(QueryOptionKey.INDEX_SKIP_CONFIG); - if (indexSkipConfigStr == null) { + public static Map> getSkipIndexes(Map queryOptions) { + // Example config: skipIndexes='col1=inverted,range&col2=inverted' + String skipIndexesStr = queryOptions.get(QueryOptionKey.SKIP_INDEXES); + if (skipIndexesStr == null) { return null; } - String[] perColumnIndexSkip = StringUtils.split(indexSkipConfigStr, '&'); - Map> indexSkipConfig = new HashMap<>(); + String[] perColumnIndexSkip = StringUtils.split(skipIndexesStr, '&'); + Map> skipIndexes = new HashMap<>(); for (String columnConf : perColumnIndexSkip) { String[] conf = StringUtils.split(columnConf, '='); if (conf.length != 2) { - throw new RuntimeException("Invalid format for " + QueryOptionKey.INDEX_SKIP_CONFIG - + ". Example of valid format: SET indexSkipConfig='col1=inverted,range&col2=inverted'"); + throw new RuntimeException("Invalid format for " + QueryOptionKey.SKIP_INDEXES + + ". Example of valid format: SET skipIndexes='col1=inverted,range&col2=inverted'"); } String columnName = conf[0]; String[] indexTypes = StringUtils.split(conf[1], ','); for (String indexType : indexTypes) { - indexSkipConfig.computeIfAbsent(columnName, k -> new HashSet<>()) + skipIndexes.computeIfAbsent(columnName, k -> new HashSet<>()) .add(FieldConfig.IndexType.valueOf(indexType.toUpperCase())); } } - return indexSkipConfig; + return skipIndexes; } @Nullable diff --git a/pinot-common/src/test/java/org/apache/pinot/common/utils/config/QueryOptionsUtilsTest.java b/pinot-common/src/test/java/org/apache/pinot/common/utils/config/QueryOptionsUtilsTest.java index d23b98fa7b17..4f0985569b49 100644 --- a/pinot-common/src/test/java/org/apache/pinot/common/utils/config/QueryOptionsUtilsTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/common/utils/config/QueryOptionsUtilsTest.java @@ -24,7 +24,6 @@ import java.util.Set; import org.apache.pinot.spi.config.table.FieldConfig; import org.apache.pinot.spi.utils.CommonConstants; -import org.apache.pinot.sql.parsers.parser.ParseException; import org.testng.Assert; import org.testng.annotations.Test; @@ -48,23 +47,22 @@ public void shouldConvertCaseInsensitiveMapToUseCorrectValues() { } @Test - public void testIndexSkipConfigParsing() - throws ParseException { - String indexSkipConfigStr = "col1=inverted,range&col2=sorted"; + public void testSkipIndexesParsing() { + String skipIndexesStr = "col1=inverted,range&col2=sorted"; Map queryOptions = - Map.of(CommonConstants.Broker.Request.QueryOptionKey.INDEX_SKIP_CONFIG, indexSkipConfigStr); - Map> indexSkipConfig = QueryOptionsUtils.getIndexSkipConfig(queryOptions); - Assert.assertEquals(indexSkipConfig.get("col1"), + Map.of(CommonConstants.Broker.Request.QueryOptionKey.SKIP_INDEXES, skipIndexesStr); + Map> skipIndexes = QueryOptionsUtils.getSkipIndexes(queryOptions); + Assert.assertEquals(skipIndexes.get("col1"), Set.of(FieldConfig.IndexType.RANGE, FieldConfig.IndexType.INVERTED)); - Assert.assertEquals(indexSkipConfig.get("col2"), + Assert.assertEquals(skipIndexes.get("col2"), Set.of(FieldConfig.IndexType.SORTED)); } @Test(expectedExceptions = RuntimeException.class) - public void testIndexSkipConfigParsingInvalid() { - String indexSkipConfigStr = "col1=inverted,range&col2"; + public void testSkipIndexesParsingInvalid() { + String skipIndexesStr = "col1=inverted,range&col2"; Map queryOptions = - Map.of(CommonConstants.Broker.Request.QueryOptionKey.INDEX_SKIP_CONFIG, indexSkipConfigStr); - QueryOptionsUtils.getIndexSkipConfig(queryOptions); + Map.of(CommonConstants.Broker.Request.QueryOptionKey.SKIP_INDEXES, skipIndexesStr); + QueryOptionsUtils.getSkipIndexes(queryOptions); } } diff --git a/pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java b/pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java index 5d06a79b1053..bf565e68e6f6 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java @@ -175,7 +175,7 @@ private void applyQueryOptions(QueryContext queryContext) { // Set skipScanFilterReorder queryContext.setSkipScanFilterReorder(QueryOptionsUtils.isSkipScanFilterReorder(queryOptions)); - queryContext.setIndexSkipConfig(QueryOptionsUtils.getIndexSkipConfig(queryOptions)); + queryContext.setSkipIndexes(QueryOptionsUtils.getSkipIndexes(queryOptions)); // Set maxExecutionThreads int maxExecutionThreads; diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java b/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java index c2829b817566..22e25eadf37a 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java @@ -126,7 +126,7 @@ public class QueryContext { // Whether server returns the final result private boolean _serverReturnFinalResult; // Collection of index types to skip per column - private Map> _indexSkipConfig; + private Map> _skipIndexes; private QueryContext(@Nullable String tableName, @Nullable QueryContext subquery, List selectExpressions, boolean distinct, List aliasList, @@ -432,15 +432,15 @@ public String toString() { + ", _expressionOverrideHints=" + _expressionOverrideHints + ", _explain=" + _explain + '}'; } - public void setIndexSkipConfig(Map> indexSkipConfig) { - _indexSkipConfig = indexSkipConfig; + public void setSkipIndexes(Map> skipIndexes) { + _skipIndexes = skipIndexes; } public boolean isIndexUseAllowed(String columnName, FieldConfig.IndexType indexType) { - if (_indexSkipConfig == null) { + if (_skipIndexes == null) { return true; } - return !_indexSkipConfig.getOrDefault(columnName, Collections.EMPTY_SET).contains(indexType); + return !_skipIndexes.getOrDefault(columnName, Collections.EMPTY_SET).contains(indexType); } public boolean isIndexUseAllowed(DataSource dataSource, FieldConfig.IndexType indexType) { diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java index 8c62a0e4dd04..45ed49343cc4 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java @@ -94,7 +94,7 @@ import static org.apache.pinot.common.function.scalar.StringFunctions.*; import static org.apache.pinot.controller.helix.core.PinotHelixResourceManager.EXTERNAL_VIEW_CHECK_INTERVAL_MS; import static org.apache.pinot.controller.helix.core.PinotHelixResourceManager.EXTERNAL_VIEW_ONLINE_SEGMENTS_MAX_WAIT_MS; -import static org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey.INDEX_SKIP_CONFIG; +import static org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey.SKIP_INDEXES; import static org.testng.Assert.*; @@ -3287,12 +3287,12 @@ public void testBooleanAggregation() testQuery("SELECT BOOL_OR(CAST(Diverted AS BOOLEAN)) FROM mytable"); } - private String buildIndexSkipConfig(String columnsAndIndexes) { - return "SET " + INDEX_SKIP_CONFIG + "='" + columnsAndIndexes + "'; "; + private String buildSkipIndexesOption(String columnsAndIndexes) { + return "SET " + SKIP_INDEXES + "='" + columnsAndIndexes + "'; "; } @Test(dataProvider = "useBothQueryEngines") - public void testIndexSkipConfig(boolean useMultiStageQueryEngine) + public void testSkipIndexes(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); long numTotalDocs = getCountStarResult(); @@ -3306,32 +3306,32 @@ public void testIndexSkipConfig(boolean useMultiStageQueryEngine) assertEquals(postQuery(TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); // disallow use of range index on DivActualElapsedTime, inverted should be unaffected - String indexSkipConf = buildIndexSkipConfig("DivActualElapsedTime=range"); + String skipIndexes = buildSkipIndexesOption("DivActualElapsedTime=range"); assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); + skipIndexes + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); + skipIndexes + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); // disallow use of inverted index on DivActualElapsedTime, range should be unaffected - indexSkipConf = buildIndexSkipConfig("DivActualElapsedTime=inverted"); + skipIndexes = buildSkipIndexesOption("DivActualElapsedTime=inverted"); // Confirm that inverted index is not used - assertFalse(postQuery(indexSkipConf + " EXPLAIN PLAN FOR " + TEST_UPDATED_INVERTED_INDEX_QUERY).toString() + assertFalse(postQuery(skipIndexes + " EXPLAIN PLAN FOR " + TEST_UPDATED_INVERTED_INDEX_QUERY).toString() .contains("FILTER_INVERTED_INDEX")); // EQ predicate type allows for using range index if one exists, even if inverted index is skipped. That is why // we still see no docs scanned even though we skip the inverted index. This is a good test to show that using - // the indexSkipConfig can allow fine-grained experimentation of index usage at query time. + // the skipIndexes can allow fine-grained experimentation of index usage at query time. assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); + skipIndexes + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); + skipIndexes + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L); // disallow use of both range and inverted indexes on DivActualElapsedTime, neither should be used at query time - indexSkipConf = buildIndexSkipConfig("DivActualElapsedTime=inverted,range"); + skipIndexes = buildSkipIndexesOption("DivActualElapsedTime=inverted,range"); assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); + skipIndexes + TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); assertEquals(postQuery( - indexSkipConf + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); + skipIndexes + TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), numTotalDocs); // Update table config to remove the new indexes, and check if the new indexes are removed TableConfig tableConfig = getOfflineTableConfig(); diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java index 3131e2b42b23..8a5ac5a9550c 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java @@ -360,7 +360,7 @@ public static class QueryOptionKey { public static final String SERVER_RETURN_FINAL_RESULT = "serverReturnFinalResult"; // Reorder scan based predicates based on cardinality and number of selected values public static final String AND_SCAN_REORDERING = "AndScanReordering"; - public static final String INDEX_SKIP_CONFIG = "indexSkipConfig"; + public static final String SKIP_INDEXES = "skipIndexes"; public static final String ORDER_BY_ALGORITHM = "orderByAlgorithm"; From 733d17973ac677098527db527e2b7387b6ff9a9f Mon Sep 17 00:00:00 2001 From: egalpin Date: Wed, 28 Feb 2024 15:57:26 -0800 Subject: [PATCH 3/4] Adds test dependencies in OfflineClusterIntegrationTest --- .../pinot/integration/tests/OfflineClusterIntegrationTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java index 45ed49343cc4..4b76d803c54f 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java @@ -3291,7 +3291,8 @@ private String buildSkipIndexesOption(String columnsAndIndexes) { return "SET " + SKIP_INDEXES + "='" + columnsAndIndexes + "'; "; } - @Test(dataProvider = "useBothQueryEngines") + @Test(dataProvider = "useBothQueryEngines", dependsOnMethods = {"testRangeIndexTriggering", + "testInvertedIndexTriggering"}) public void testSkipIndexes(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine); From 993522269a06bf762961002a387beae5994ad87e Mon Sep 17 00:00:00 2001 From: egalpin Date: Thu, 29 Feb 2024 14:17:04 -0800 Subject: [PATCH 4/4] Revert "Adds test dependencies in OfflineClusterIntegrationTest" This reverts commit 733d17973ac677098527db527e2b7387b6ff9a9f. --- .../pinot/integration/tests/OfflineClusterIntegrationTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java index 4b76d803c54f..45ed49343cc4 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java @@ -3291,8 +3291,7 @@ private String buildSkipIndexesOption(String columnsAndIndexes) { return "SET " + SKIP_INDEXES + "='" + columnsAndIndexes + "'; "; } - @Test(dataProvider = "useBothQueryEngines", dependsOnMethods = {"testRangeIndexTriggering", - "testInvertedIndexTriggering"}) + @Test(dataProvider = "useBothQueryEngines") public void testSkipIndexes(boolean useMultiStageQueryEngine) throws Exception { setUseMultiStageQueryEngine(useMultiStageQueryEngine);