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



##########
File path: 
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableImplV2V3.java
##########
@@ -167,6 +178,18 @@ public DataTableImplV2(ByteBuffer byteBuffer)
       _variableSizeDataBytes = null;
       _variableSizeData = null;
     }
+
+    // Read positional data.
+    String[] positionalData = null;
+    if (version == VERSION_3 && byteBuffer.hasRemaining()) {
+      int positionalDataStart = variableSizeDataStart + variableSizeDataLength;
+      int positionalDataLength = byteBuffer.remaining();
+      byteBuffer.position(positionalDataStart);

Review comment:
       Since we are using `byteBuffer.remaining() `to compute the length of 
positional data, it implies we are treating it as a **footer** of specific 
format (name-value pairs as defined in the enum) even though we are not calling 
it. So, technically no other structure can come after this as we will fail to 
distinguish between the length of positional data + whatever comes after it. I 
don't think we should limit that flexibility. Even if we call this footer, 
let's please write the length of footer as well before line 348




-- 
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:
us...@infra.apache.org



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

Reply via email to