mayankshriv commented on a change in pull request #7595:
URL: https://github.com/apache/pinot/pull/7595#discussion_r734093576
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java
##########
@@ -207,7 +207,7 @@ private void convertColumn(FieldSpec fieldSpec)
int numDocs = _originalSegmentMetadata.getTotalDocs();
int lengthOfLongestEntry =
_originalSegmentMetadata.getColumnMetadataFor(columnName).getColumnMaxLength();
try (ForwardIndexCreator rawIndexCreator = SegmentColumnarIndexCreator
- .getRawIndexCreatorForColumn(_convertedIndexDir,
ChunkCompressionType.SNAPPY, columnName, storedType, numDocs,
+ .getRawIndexCreatorForSVColumn(_convertedIndexDir,
ChunkCompressionType.SNAPPY, columnName, storedType, numDocs,
Review comment:
The converter should fail gracefully upfront for MV columns?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/VarByteChunkSVForwardIndexWriter.java
##########
@@ -33,27 +33,29 @@
* The layout of the file is as follows:
* <p> Header Section: </p>
* <ul>
- * <li> Integer: File format version. </li>
- * <li> Integer: Total number of chunks. </li>
- * <li> Integer: Number of docs per chunk. </li>
- * <li> Integer: Length of longest entry (in bytes). </li>
- * <li> Integer: Total number of docs (version 2 onwards). </li>
- * <li> Integer: Compression type enum value (version 2 onwards). </li>
- * <li> Integer: Start offset of data header (version 2 onwards). </li>
- * <li> Integer array: Integer offsets for all chunks in the data (upto
version 2),
- * Long array: Long offsets for all chunks in the data (version 3 onwards)
</li>
+ * <li> Integer: File format version. </li>
Review comment:
Why lose the indentation? Is this yet another style check difference?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/BaseChunkSVForwardIndexWriter.java
##########
@@ -151,8 +154,11 @@ private int writeHeader(ChunkCompressionType
compressionType, int totalDocs, int
int dataHeaderStart = offset + Integer.BYTES;
_header.putInt(dataHeaderStart);
}
+ }
- return headerSize;
+ private static int headerSize(int totalDocs, int numDocsPerChunk, int
headerEntryChunkOffsetSize) {
Review comment:
IIRC, the reason why I had put the headerSize calculation so that in
future if someone changes the header they don't miss to update the headerSize
(easier to miss if a separate method). Any reason you prefer the latter?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
