[GitHub] [incubator-pinot] snleee commented on a change in pull request #5470: Derive numDocsPerChunk for var byte raw index from metadata only if config is enabled.

[GitBox <gi...@apache.org>]

snleee commented on a change in pull request #5470:
URL: https://github.com/apache/incubator-pinot/pull/5470#discussion_r432895436



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -330,8 +330,32 @@ public long getLatestIngestionTimestamp() {
    */
   private boolean isNoDictionaryColumn(Set<String> noDictionaryColumns, Set<String> invertedIndexColumns,
       Set<String> textIndexColumns, FieldSpec fieldSpec, String column) {
-    return textIndexColumns.contains(column) || (noDictionaryColumns.contains(column) && fieldSpec.isSingleValueField()
-        && !invertedIndexColumns.contains(column));
+    if (textIndexColumns.contains(column)) {
+      // text column is no dictionary currently
+      return true;
+    }
+    FieldSpec.DataType dataType = fieldSpec.getDataType();
+    if (noDictionaryColumns.contains(column)) {
+      // Earlier we didn't support noDict in consuming segments for STRING and BYTES columns.
+      // So even if the user had the column in noDictionaryColumns set in table config, we still
+      // created dictionary in consuming segments.
+      // Later on we added this support. There is a particular impact of this change on the use cases
+      // that have set noDict on their STRING dimension columns for other performance
+      // reasons and also want metricsAggregation. These use cases don't get to
+      // aggregateMetrics because the new implementation is able to honor their table config setting
+      // of noDict on STRING/BYTES. Without metrics aggregation, memory pressure increases.
+      // So to continue aggregating metrics for such cases, we will create dictionary even
+      // if the column is part of noDictionary set from table config
+      if (fieldSpec instanceof DimensionFieldSpec && _aggregateMetrics && (dataType == FieldSpec.DataType.STRING ||

Review comment:
       What was the original behavior before your recent change? Did we explicitly check `STRING` and `BYTES` types also?

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -193,9 +194,10 @@ public void init(SegmentGeneratorConfig segmentCreationSpec, SegmentIndexCreatio
             getColumnCompressionType(segmentCreationSpec, fieldSpec);
 
         // Initialize forward index creator
+        boolean deriveNumChunksForVarByteRawIndex = shouldDeriveNumChunksForRawIndex(columnName, segmentCreationSpec.getColumnProperties());

Review comment:
       `deriveNumDocsPerChunk` sounds more accurate? (applicable to all related config & variable names)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org