[GitHub] [pinot] jackjlli commented on a diff in pull request #9424: refactor LLRealtimeSegmentDataManager

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

jackjlli commented on code in PR #9424:
URL: https://github.com/apache/pinot/pull/9424#discussion_r974667348


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/ColumnDescriptionsContainer.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.pinot.segment.local.realtime.converter;

Review Comment:
   Include the header into this class.



##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1528,8 +1520,8 @@ private void makeStreamMetadataProvider(String reason) {
     if (_streamMetadataProvider != null) {
       closeStreamMetadataProvider();
     }
-    _segmentLogger.info("Creating new stream metadata provider, reason: {}", reason);
-    _streamMetadataProvider = _streamConsumerFactory.createStreamMetadataProvider(_clientId);
+    _segmentLogger.info("Creating new partition metadata provider, reason: {}", reason);
+    _streamMetadataProvider = _streamConsumerFactory.createPartitionMetadataProvider(_clientId, _partitionGroupId);

Review Comment:
   Should we consider renaming the caller method name?



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/RealtimeSegmentConverter.java:
##########
@@ -49,34 +49,26 @@ public class RealtimeSegmentConverter {
   private final String _segmentName;
   private final String _sortedColumn;
   private final List<String> _invertedIndexColumns;
-  private final List<String> _textIndexColumns;
-  private final List<String> _fstIndexColumns;
-  private final List<String> _noDictionaryColumns;
-  private final List<String> _varLengthDictionaryColumns;
+  private final ColumnDescriptionsContainer _columnDescriptionsContainer;
   private final boolean _nullHandlingEnabled;
 
   public RealtimeSegmentConverter(MutableSegmentImpl realtimeSegment, SegmentZKPropsConfig segmentZKPropsConfig,
       String outputPath, Schema schema, String tableName, TableConfig tableConfig, String segmentName,
-      String sortedColumn, List<String> invertedIndexColumns, List<String> textIndexColumns,
-      List<String> fstIndexColumns, List<String> noDictionaryColumns, List<String> varLengthDictionaryColumns,
-      boolean nullHandlingEnabled) {
+      ColumnDescriptionsContainer cdc, boolean nullHandlingEnabled) {
     _realtimeSegmentImpl = realtimeSegment;
     _segmentZKPropsConfig = segmentZKPropsConfig;
     _outputPath = outputPath;
-    _invertedIndexColumns = new ArrayList<>(invertedIndexColumns);
-    if (sortedColumn != null) {
-      _invertedIndexColumns.remove(sortedColumn);
+    _columnDescriptionsContainer = cdc;
+    _invertedIndexColumns = new ArrayList<>(_columnDescriptionsContainer.getInvertedIndexColumns());
+    if (cdc.getSortedColumn() != null) {
+      _invertedIndexColumns.remove(cdc.getSortedColumn());
     }
     _dataSchema = getUpdatedSchema(schema);
-    _sortedColumn = sortedColumn;
+    _sortedColumn = cdc.getSortedColumn();
     _tableName = tableName;
     _tableConfig = tableConfig;
     _segmentName = segmentName;
-    _noDictionaryColumns = noDictionaryColumns;

Review Comment:
   I think we should either adding an instance level variable just for columnDescriptionsContainer object, or assigning all the necessary indices into instance variables, instead of keeping both here. Otherwise, it's a bit harder to maintain in the future.



##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/HLRealtimeSegmentDataManager.java:
##########
@@ -284,11 +285,12 @@ public void run() {
           _segmentLogger.info("Indexed {} raw events", _realtimeSegment.getNumDocsIndexed());
           File tempSegmentFolder = new File(_resourceTmpDir, "tmp-" + System.currentTimeMillis());
           // lets convert the segment now
+          ColumnDescriptionsContainer cdc = new ColumnDescriptionsContainer(_sortedColumn, _invertedIndexColumns,

Review Comment:
   nit: it'd be good to move this before the comments in Line 287? Also, use `columnDescriptionsContainer` as the variable name instead?



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/ColumnDescriptionsContainer.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.pinot.segment.local.realtime.converter;
+
+import java.util.List;
+
+
+public class ColumnDescriptionsContainer {

Review Comment:
   nit: maybe coming up with a better name like `ColumnIndicesForRealtimeTable` and the like, as it's just for realtime table?



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/ColumnDescriptionsContainer.java:
##########
@@ -0,0 +1,48 @@
+package org.apache.pinot.segment.local.realtime.converter;
+
+import java.util.List;
+
+
+public class ColumnDescriptionsContainer {

Review Comment:
   Add javadoc into this class.



-- 
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-unsubscribe@pinot.apache.org

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