gortiz commented on code in PR #16025:
URL: https://github.com/apache/pinot/pull/16025#discussion_r2132171879
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java:
##########
@@ -41,7 +41,15 @@ public abstract class AbstractIndexType<C extends
IndexConfig, IR extends IndexR
private ColumnConfigDeserializer<C> _deserializer;
private IndexReaderFactory<IR> _readerFactory;
- protected abstract ColumnConfigDeserializer<C> createDeserializer();
+ protected ColumnConfigDeserializer<C> createDeserializer() {
+ ColumnConfigDeserializer<C> fromIndexes =
+ IndexConfigDeserializer.fromIndexes(getPrettyName(),
getIndexConfigClass());
+ return
fromIndexes.withExclusiveAlternative(createDeserializerNotFromIndexes());
+ }
+
+ protected ColumnConfigDeserializer<C> createDeserializerNotFromIndexes() {
+ throw new UnsupportedOperationException("Please override
createDeserializer or createDeserializerNotFromIndexes");
+ }
Review Comment:
I don't think this default implementation is a good idea. This makes it more
difficult to implement a new index whose deserializer should only read the
`indexes` config, as they will need to override this new method in order not to
fail.
Instead I would recommend to make this method `@Nullable` and return null
by default. Then `createDeserializer` could be implemented as:
```java
protected ColumnConfigDeserializer<C> createDeserializer() {
ColumnConfigDeserializer<C> fromIndexes =
IndexConfigDeserializer.fromIndexes(getPrettyName(),
getIndexConfigClass());
ColumnConfigDeserializer<C> alternative =
createDeserializerNotFromIndexes();
if (alternative == null) {
return fromIndexes;
}
return fromIndexes.withExclusiveAlternative(alternative);
}
```
That way should be similar, but no override will be needed in the simpler
cases. This is specially important because the compiler will not fail in case
neither of these two methods are overload, so the failure will be found at
runtime.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java:
##########
@@ -79,19 +79,18 @@ public String getPrettyName() {
}
@Override
- public ColumnConfigDeserializer<IndexConfig> createDeserializer() {
+ public ColumnConfigDeserializer<IndexConfig>
createDeserializerNotFromIndexes() {
return (TableConfig tableConfig, Schema schema) -> {
Collection<FieldSpec> allFieldSpecs = schema.getAllFieldSpecs();
Map<String, IndexConfig> configMap =
Maps.newHashMapWithExpectedSize(allFieldSpecs.size());
- boolean enableColumnBasedNullHandling =
schema.isEnableColumnBasedNullHandling();
- boolean nullHandlingEnabled = tableConfig.getIndexingConfig() != null
- && tableConfig.getIndexingConfig().isNullHandlingEnabled();
+ boolean columnBasedNullHandlingEnabled =
schema.isEnableColumnBasedNullHandling();
+ boolean nullHandlingEnabled =
tableConfig.getIndexingConfig().isNullHandlingEnabled();
Review Comment:
Are we sure `tableConfig.getIndexingConfig() != null ` here?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexType.java:
##########
@@ -69,6 +70,10 @@ default BuildLifecycle getIndexBuildLifecycle() {
Map<String, C> getConfig(TableConfig tableConfig, Schema schema);
+ /// Validates if the index config is valid for the given field spec and
other index configs.
Review Comment:
I think it would be better to return an object containing the validation
errors. It could be a new class or a List<String>. This would be more explicit
and would allow us to display more than one error at the same time.
If we keep the exception model, I suggest documenting here that the expected
exception to be thrown is IllegalStateException.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
