[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #5285: Use 8byte offsets in chunk based raw index creator

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

siddharthteotia commented on a change in pull request #5285:
URL: https://github.com/apache/incubator-pinot/pull/5285#discussion_r413276735



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/io/reader/impl/v1/BaseChunkSingleValueReader.java
##########
@@ -47,6 +48,7 @@
   protected final int _numDocsPerChunk;
   protected final int _numChunks;
   protected final int _lengthOfLongestEntry;
+  private final int _version;

Review comment:
       Done. 
   
   @mcvsubbu , this actually comes handy now itself since I haven't bumped the version of fixed byte chunk writer. It is still on version 2 and using 4-byte chunk offset entries in file header. So the current changes protect compatibility of v1/v2 for var-byte, read/write new var byte in v3 and still continue to read/write fixed byte indexes in v1/v2.
   
   I am having mixed opinions on bumping up the version of fixed byte chunk writer to use 8byte offsets as well. The thing is that if we don't bump it up and tomorrow file format for fixed byte changes (for some reason), then we will bump it up to 3. At that time  it will automatically get 8-byte offsets by virtue of being at version >=3. So may be do it now and keep the versions same.
   
   The flip side is that you would ideally want to evolve fixed-byte and var-byte formats independently (which is what is done in this PR by keeping the fixed byte writer still at version 2). Obviously if we separate out base class and duplicate code, then things will be simplified but that's not the best option. Thoughts?
   

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/io/writer/impl/v1/BaseChunkSingleValueWriter.java
##########
@@ -45,7 +47,7 @@
   protected final ChunkCompressor _chunkCompressor;
 
   protected int _chunkSize;
-  protected int _dataOffset;
+  protected long _dataOffset;

Review comment:
       done

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/io/writer/impl/v1/BaseChunkSingleValueWriter.java
##########
@@ -37,6 +37,8 @@
  */
 public abstract class BaseChunkSingleValueWriter implements SingleColumnSingleValueWriter {
   private static final Logger LOGGER = LoggerFactory.getLogger(BaseChunkSingleValueWriter.class);
+  public static final int FILE_HEADER_ENTRY_CHUNK_OFFSET_SIZE_V1V2 = Integer.BYTES;
+  public static final int FILE_HEADER_ENTRY_CHUNK_OFFSET_SIZE = Long.BYTES;

Review comment:
       done

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/io/writer/impl/v1/BaseChunkSingleValueWriter.java
##########
@@ -139,7 +141,8 @@ public void close()
   private int writeHeader(ChunkCompressorFactory.CompressionType compressionType, int totalDocs, int numDocsPerChunk,
       int sizeOfEntry, int version) {
     int numChunks = (totalDocs + numDocsPerChunk - 1) / numDocsPerChunk;
-    int headerSize = (numChunks + 7) * Integer.BYTES; // 7 items written before chunk indexing.
+    // 7 items written before chunk indexing.
+    int headerSize = (7 * Integer.BYTES) + (numChunks * VarByteChunkSingleValueWriter.FILE_HEADER_ENTRY_CHUNK_OFFSET_SIZE);

Review comment:
       I agree. done




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