somandal commented on code in PR #16002:
URL: https://github.com/apache/pinot/pull/16002#discussion_r2178584430
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -477,6 +477,9 @@ private void rewriteForwardIndexForCompressionChange(String
column, ColumnMetada
try (ForwardIndexReader<?> reader = ForwardIndexType.read(segmentWriter,
columnMetadata)) {
IndexCreationContext.Builder builder =
IndexCreationContext.builder().withIndexDir(indexDir).withColumnMetadata(columnMetadata);
+ if (_tableConfig != null) {
Review Comment:
So the issue is that all the interfaces mark the `tableConfig` as
`@Nullable`, e.g:
```
public BaseIndexHandler(SegmentDirectory segmentDirectory, Map<String,
FieldIndexConfigs> fieldIndexConfigs,
@Nullable TableConfig tableConfig)
```
Similarly:
```
IndexHandler createIndexHandler(SegmentDirectory segmentDirectory,
Map<String, FieldIndexConfigs> configsByCol,
@Nullable Schema schema, @Nullable TableConfig tableConfig);
```
And some code paths that are just meant to read and tests can set the
`tableConfig` as null:
```
/**
* Loads the segment with empty schema and IndexLoadingConfig. This method
is used to
* access the segment without modifying it, i.e. in read-only mode.
*/
public static ImmutableSegment load(File indexDir, ReadMode readMode)
throws Exception {
IndexLoadingConfig defaultIndexLoadingConfig = new IndexLoadingConfig();
defaultIndexLoadingConfig.setReadMode(readMode);
return load(indexDir, defaultIndexLoadingConfig, false);
}
/**
* NOTE: Can be used in production code when we want to load a segment as
is without any modifications.
*/
public IndexLoadingConfig() {
this(null, null, null); // null tableConfig
}
public IndexLoadingConfig(@Nullable InstanceDataManagerConfig
instanceDataManagerConfig,
@Nullable TableConfig tableConfig, @Nullable Schema schema) {
_instanceDataManagerConfig = instanceDataManagerConfig;
_tableConfig = tableConfig;
_schema = schema;
init();
}
```
So that's why I decided to add handling throughout to prevent weird issues
with tests and all. I don't think for the code paths where we actually intend
to create segments we'll come across a null tableConfig. Even the
tableNameWithType null checks are mostly for this kind of compliance. Let me
know if you think we should remove this null handling and try it out?
--
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
