[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6874: Init mutableSegmentRecordReader to handle partial upsert record lookup

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

Jackie-Jiang commented on a change in pull request #6874:
URL: https://github.com/apache/incubator-pinot/pull/6874#discussion_r625414196



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/readers/PinotSegmentRecordReader.java
##########
@@ -110,7 +136,7 @@ public PinotSegmentRecordReader(File indexDir, @Nullable Schema schema, @Nullabl
         _docIdsInSortedColumnOrder = null;
       }
     } catch (Exception e) {
-      _immutableSegment.destroy();
+      _indexSegment.destroy();

Review comment:
       We don't want to destroy the segment if it is passed in instead of loaded in this class

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/readers/PinotSegmentRecordReader.java
##########
@@ -166,6 +192,6 @@ public void close()
     for (PinotSegmentColumnReader columnReader : _columnReaderMap.values()) {
       columnReader.close();
     }
-    _immutableSegment.destroy();
+    _indexSegment.destroy();

Review comment:
       We don't want to destroy the segment if it is passed in instead of loaded in this class

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/readers/PinotSegmentRecordReader.java
##########
@@ -62,6 +65,15 @@ public PinotSegmentRecordReader(File indexDir)
     this(indexDir, null, null);
   }
 
+  /**
+   * Read records using the segment schema
+   * @param mutableSegment mutable segment index
+   */
+  public PinotSegmentRecordReader(MutableSegment mutableSegment)

Review comment:
       Pass in `IndexSegment` for flexibility

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -160,6 +161,8 @@
   private final ThreadSafeMutableRoaringBitmap _validDocIds;
   private final ValidDocIndexReader _validDocIndex;
 
+  private final PinotSegmentRecordReader _pinotSegmentRecordReader;

Review comment:
       I don't think we need to initialize a record reader within the mutable segment. How are you planning to use it?




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