wgtmac commented on code in PR #3567:
URL: https://github.com/apache/parquet-java/pull/3567#discussion_r4254386433
##########
parquet-column/src/main/java/org/apache/parquet/column/values/delta/DeltaBinaryPackingValuesWriterForInteger.java:
##########
@@ -116,18 +122,16 @@ private void flushBlockBuffer() {
for (int i = 0; i < miniBlocksToFlush; i++) {
// writing i th miniblock
int currentBitWidth = bitWidths[i];
- int blockOffset = 0;
- BytePacker packer = Packer.LITTLE_ENDIAN.newBytePacker(currentBitWidth);
- int miniBlockStart = i * config.miniBlockSizeInValues;
- for (int j = miniBlockStart; j < (i + 1) * config.miniBlockSizeInValues;
j += 8) { // 8 values per pack
- // mini block is atomic in terms of flushing
- // This may write more values when reach to the end of data writing to
last mini block,
- // since it may not be aligned to miniblock,
- // but doesn't matter. The reader uses total count to see if reached
the end.
- packer.pack8Values(deltaBlockBuffer, j, miniBlockByteBuffer,
blockOffset);
- blockOffset += currentBitWidth;
+ BytePacker packer = packerCache[currentBitWidth];
+ if (packer == null) {
+ packer = Packer.LITTLE_ENDIAN.newBytePacker(currentBitWidth);
+ packerCache[currentBitWidth] = packer;
}
- baos.write(miniBlockByteBuffer, 0, blockOffset);
+ int miniBlockStart = i * config.miniBlockSizeInValues;
+ // Mini blocks are always flushed as full 32-value groups in the current
format.
+ // Use the packer's 32-value entry point to avoid four pack8Values calls
per miniblock.
+ packer.pack32Values(deltaBlockBuffer, miniBlockStart,
miniBlockByteBuffer, 0);
Review Comment:
Miniblock sizes can be any multiple of 32. With `blockSizeInValues=256` and
`miniBlockNum=4`, each miniblock contains 64 values, but this writes only 32,
producing a corrupt page. The long writer has the same issue. This needs to
loop over the entire miniblock, with a round-trip test for a 64-value miniblock.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/delta/DeltaBinaryPackingValuesWriter.java:
##########
@@ -76,11 +76,6 @@ public abstract class DeltaBinaryPackingValuesWriter extends
ValuesWriter {
*/
protected byte[] miniBlockByteBuffer;
- // TODO: remove this.
- public DeltaBinaryPackingValuesWriter(int slabSize, int pageSize,
ByteBufferAllocator allocator) {
Review Comment:
This constructor was released in 1.18.0. Removing it breaks external
subclasses calling `super(slabSize, pageSize, allocator)` and can cause
`NoSuchMethodError`. The japicmp exclusion only hides the incompatibility. Keep
the constructor; removing it is unrelated to the performance changes.
--
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]
