gortiz commented on code in PR #10653:
URL: https://github.com/apache/pinot/pull/10653#discussion_r1177416617
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java:
##########
@@ -67,14 +67,11 @@ public IndexConfig getDefaultConfig() {
@Override
public ColumnConfigDeserializer<IndexConfig> createDeserializer() {
- return IndexConfigDeserializer.fromIndexes("null", getIndexConfigClass())
- .withFallbackAlternative(
Review Comment:
It is one of the options on `MergedColumnConfigDeserializer`. Specifically,
it uses `MergedColumnConfigDeserializer.OnConflict.PICK_FIRST`, which means
that in case a configuration is specified twice, it will pick the first and
silently skip the second.
In this case it meant that first it would look for something like:
```js
{
fieldConfigList: [
{
name: "whatever",
indexes: {
null: {} // this is what IndexConfigDeserializer.fromIndexes looks
for
}
}
}
}
```
And if it is not found, it would look for the alternative which is to read
`tableConfig.getIndexingConfig().isNullHandlingEnabled()`. To be more precise,
both alternatives will always be read and then merged silently. That means that
in case the config is incorrectly specified in one of the branches, we are
going to produce a failure.
The new code removes the first part in order to do not look for nullability
in `fieldConfigList`, which is desired because it is actually not supported, so
we would be generating an extra index that would never be used.
--
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]
