Jackie-Jiang commented on code in PR #15283:
URL: https://github.com/apache/pinot/pull/15283#discussion_r2011061402
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1003,6 +1003,7 @@ private static void validateIndexingConfig(IndexingConfig
indexingConfig, @Nulla
if (indexingConfig.getBloomFilterConfigs() != null) {
bloomFilterColumns.addAll(indexingConfig.getBloomFilterConfigs().keySet());
}
+
Review Comment:
(nit) Revert empty line since it is connected with previous lines' logic
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1079,6 +1080,23 @@ private static void
validateIndexingConfig(IndexingConfig indexingConfig, @Nulla
}
}
+ // Bloom index semantic validation
+ // Bloom filter cannot be defined on boolean columns
+ if (indexingConfig.getBloomFilterColumns() != null) {
+ for (String bloomIndexCol : indexingConfig.getBloomFilterColumns()) {
+ Preconditions.checkState(
+ schema.getFieldSpecFor(bloomIndexCol).getDataType() !=
FieldSpec.DataType.BOOLEAN,
+ "Cannot create a bloom filter on boolean column " + bloomIndexCol);
+ }
+ }
+ if (indexingConfig.getBloomFilterConfigs() != null) {
+ for (String bloomIndexCol:
indexingConfig.getBloomFilterConfigs().keySet()) {
+ Preconditions.checkState(
+ schema.getFieldSpecFor(bloomIndexCol).getDataType() !=
FieldSpec.DataType.BOOLEAN,
+ "Cannot create a bloom filter on boolean column " + bloomIndexCol);
+ }
+ }
+
Review Comment:
Move this logic to line 1001 and 1004 for consistency
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/TableIndexingTest.java:
##########
@@ -297,6 +297,9 @@ public void testAddIndex(TestCase testCase) {
...
} */
// no params
+ if (field.getDataType() == DataType.BOOLEAN) {
+ throw new IllegalArgumentException("Bloom filter index is not
supported for BOOLEAN type");
+ }
Review Comment:
Should we add this check into the test? Shouldn't it be thrown from the
production code to be verified?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1242,6 +1260,12 @@ private static void validateFieldConfigList(TableConfig
tableConfig, @Nullable S
// Validate the forward index disabled compatibility with other indexes
if enabled for this column
validateForwardIndexDisabledIndexCompatibility(columnName, fieldConfig,
indexingConfig, schema, tableType);
+ // Validate bloom filter is not added to boolean column
+ if (fieldConfig.getIndexes() != null &&
fieldConfig.getIndexes().has("bloom")) {
Review Comment:
Move this check into the following switch case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
