swaminathanmanish commented on code in PR #10557:
URL: https://github.com/apache/pinot/pull/10557#discussion_r1159288965
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -309,8 +309,16 @@ Map<String, List<Operation>>
computeOperations(SegmentDirectory.Reader segmentRe
LOGGER.warn("Cannot enable dictionary for column={} as schema or
tableConfig is null.", column);
continue;
}
-
- columnOperationsMap.put(column,
Collections.singletonList(Operation.ENABLE_DICTIONARY));
+ ColumnMetadata existingColumnMetadata =
_segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
Review Comment:
> Looks like even for `forwardIndexDisabled`, the optimize configs can be
set. Can you either add this check for the `forwardIndexDisabled` code paths as
well, or add some code to the
`TableConfigUtils::validateForwardIndexDisabledIndexCompatibility()` to
disallow setting the optimizer configs when the forward index is disabled. I
think the latter may make more sense since if the forward index is disabled
then there is no point in applying this optimization and there are other
complications with not having a dictionary.
>
> I'd make this decision based on whether the main reason for these optimize
configs is to keep the forward index small or whether to avoid the dictionary
creation. I believe it's for the former, but confirm with @KKcorps as well.
>
> Please add tests for the scenario as well.
This seems to be the reason from the
[PR](https://github.com/apache/pinot/pull/8398).
<<For high cardinality metric columns (especialy doubles/floats), dictionary
may be an overhead not only for **storage space** but also it **adds one
additional hop for read**.>>
--
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]
