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