Jackie-Jiang commented on code in PR #13994:
URL: https://github.com/apache/pinot/pull/13994#discussion_r1765674170
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/dictionary/DictionaryIndexType.java:
##########
@@ -218,16 +218,17 @@ public static boolean
shouldUseVarLengthDictionary(FieldSpec.DataType columnStor
* This function evaluates whether to override dictionary (i.e use
noDictionary)
* for a column even when its explicitly configured. This evaluation is for
both dimension and metric
* column types.
+ *
+ * @return true if dictionary should be created, false if noDictionary
should be used
*/
- public static boolean ignoreDictionaryOverride(boolean optimizeDictionary,
- boolean optimizeDictionaryForMetrics, double
noDictionarySizeRatioThreshold,
- FieldSpec fieldSpec, FieldIndexConfigs fieldIndexConfigs, int
cardinality,
- int totalNumberOfEntries) {
- // For an inverted index dictionary is required
- if (fieldIndexConfigs.getConfig(StandardIndexes.inverted()).isEnabled()) {
- return true;
- }
- if (optimizeDictionary) {
+ public static boolean ignoreDictionaryOverride(boolean optimizeDictionary,
boolean optimizeDictionaryForMetrics,
+ double noDictionarySizeRatioThreshold, @Nullable Double
noDictionaryCardinalityRatioThreshold,
+ FieldSpec fieldSpec, FieldIndexConfigs fieldIndexConfigs, int
cardinality, int totalNumberOfEntries) {
+ // For an inverted index dictionary is required
Review Comment:
The indentation is incorrect
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -293,8 +293,9 @@ private boolean
createDictionaryForColumn(ColumnIndexCreationInfo info, SegmentG
FieldIndexConfigs fieldIndexConfigs =
config.getIndexConfigsByColName().get(column);
if
(DictionaryIndexType.ignoreDictionaryOverride(config.isOptimizeDictionary(),
- config.isOptimizeDictionaryForMetrics(),
config.getNoDictionarySizeRatioThreshold(), spec, fieldIndexConfigs,
- info.getDistinctValueCount(), info.getTotalNumberOfEntries())) {
+ config.isOptimizeDictionaryForMetrics(),
config.getNoDictionarySizeRatioThreshold(),
Review Comment:
This list has become quite long. Suggest adding a new config class for
dictionary tuning related configs
--
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
