You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/10 21:36:10 UTC
[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6541: - Adding new validation for Json, TEXT indexing
mayankshriv commented on a change in pull request #6541:
URL: https://github.com/apache/incubator-pinot/pull/6541#discussion_r574093157
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
##########
@@ -474,11 +478,48 @@ private static void validateIndexingConfig(@Nullable IndexingConfig indexingConf
Preconditions.checkState(schema.getFieldSpecFor(columnName) != null,
"Column Name " + columnName + " defined in " + configName + " must be a valid column defined in the schema");
}
+
+ // Range index semantic validation
+ // Range index can be defined on numeric columns and any column with a dictionary
+ if (indexingConfig.getRangeIndexColumns() != null) {
+ for (String rangeIndexCol : indexingConfig.getRangeIndexColumns()) {
+ Preconditions.checkState(
+ schema.getFieldSpecFor(rangeIndexCol).getDataType().isNumeric() || !noDictionaryColumnsSet
+ .contains(rangeIndexCol),
+ "Cannot create a range index on non-numeric/no-dictionary column " + rangeIndexCol);
+ }
+ }
+
+ // Var length dictionary semantic validation
+ if (indexingConfig.getVarLengthDictionaryColumns() != null) {
+ for (String varLenDictCol : indexingConfig.getVarLengthDictionaryColumns()) {
+ FieldSpec varLenDictFieldSpec = schema.getFieldSpecFor(varLenDictCol);
+ switch (varLenDictFieldSpec.getDataType()) {
+ case STRING:
+ case BYTES:
+ continue;
+ default:
+ throw new IllegalStateException(
+ "var length dictionary can only be created for columns of type STRING and BYTES. Invalid for column "
Review comment:
What about MV columns, do we support var length for MV columns?
Also, s/var length/Var length
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
##########
@@ -870,6 +916,69 @@ public void testValidateIndexingConfig() {
} catch (Exception e) {
// expected
}
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("non-existent-column")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for non existent column in Json index config");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("intCol")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Json index defined on non string column");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("multiValCol")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Json index defined on multi-value column");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRangeIndexColumns(columnList).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ } catch (Exception e) {
+ Assert.fail("Should work for range index defined on dictionary encoded string column");
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRangeIndexColumns(columnList)
+ .setNoDictionaryColumns(columnList).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for range index defined on non numeric/no-dictionary column");
+ } catch (Exception e) {
+ // Expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setVarLengthDictionaryColumns(Arrays.asList("intCol")).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Var length dictionary defined for non string/bytes column");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setJsonIndexColumns(Arrays.asList("multiValCol")).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Json Index defined on a multi value column");
+ } catch (Exception e) {
+ // expected
Review comment:
Same here.
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/TableConfigUtilsTest.java
##########
@@ -870,6 +916,69 @@ public void testValidateIndexingConfig() {
} catch (Exception e) {
// expected
}
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("non-existent-column")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for non existent column in Json index config");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("intCol")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Json index defined on non string column");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
+ .setJsonIndexColumns(Arrays.asList("multiValCol")).build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for Json index defined on multi-value column");
+ } catch (Exception e) {
+ // expected
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRangeIndexColumns(columnList).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ } catch (Exception e) {
+ Assert.fail("Should work for range index defined on dictionary encoded string column");
+ }
+
+ tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setRangeIndexColumns(columnList)
+ .setNoDictionaryColumns(columnList).
+ build();
+ try {
+ TableConfigUtils.validate(tableConfig, schema);
+ Assert.fail("Should fail for range index defined on non numeric/no-dictionary column");
+ } catch (Exception e) {
+ // Expected
Review comment:
Swallowing exception?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org