diff --git a/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/CodecFactory.java b/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/CodecFactory.java index 9024954bb9..dfac300a89 100644 --- a/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/CodecFactory.java +++ b/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/CodecFactory.java @@ -464,7 +464,12 @@ private String cacheKey(CompressionCodecName codecName) { private String cacheKey(CompressionCodecName codecName, int level) { String codecClass = codecName.getHadoopCompressionCodecClassName(); - return (codecClass == null ? codecName.name() : codecClass) + ":" + level; + // Use a distinct namespace ("#level=") for the leveled path so that this key can never + // collide with the no-level cacheKey(codecName), which appends the raw configuration value + // (e.g. ":5" when zlib.compress.level=5 is set directly). Both maps that use + // these keys (the per-factory compressors map and the shared static CODEC_BY_NAME) would + // otherwise return a codec built from the raw config instead of the level-configured one. + return (codecClass == null ? codecName.name() : codecClass) + "#level=" + level; } @Override diff --git a/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java b/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java index ebb1208dc0..ba423f837d 100644 --- a/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java +++ b/parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java @@ -1191,10 +1191,18 @@ private ColumnChunkPageReadStore internalReadRowGroup(int blockIndex) throws IOE } // actually read all the chunks ChunkListBuilder builder = new ChunkListBuilder(block.getRowCount()); - readAllPartsVectoredOrNormal(allParts, builder); - rowGroup.setReleaser(builder.releaser); - for (Chunk chunk : builder.build()) { - readChunkPages(chunk, block, rowGroup); + try { + readAllPartsVectoredOrNormal(allParts, builder); + rowGroup.setReleaser(builder.releaser); + for (Chunk chunk : builder.build()) { + readChunkPages(chunk, block, rowGroup); + } + } catch (RuntimeException | IOException e) { + // If we fail before the releaser is transferred to the row group (e.g. a vectored range + // times out after earlier ranges already registered their buffers), release any buffers + // that were registered so far so that partially-read row groups do not leak. + builder.releaser.close(); + throw e; } return rowGroup; @@ -1466,10 +1474,18 @@ private ColumnChunkPageReadStore internalReadFilteredRowGroup( } } } - readAllPartsVectoredOrNormal(allParts, builder); - rowGroup.setReleaser(builder.releaser); - for (Chunk chunk : builder.build()) { - readChunkPages(chunk, block, rowGroup); + try { + readAllPartsVectoredOrNormal(allParts, builder); + rowGroup.setReleaser(builder.releaser); + for (Chunk chunk : builder.build()) { + readChunkPages(chunk, block, rowGroup); + } + } catch (RuntimeException | IOException e) { + // If we fail before the releaser is transferred to the row group (e.g. a vectored range + // times out after earlier ranges already registered their buffers), release any buffers + // that were registered so far so that partially-read row groups do not leak. + builder.releaser.close(); + throw e; } return rowGroup; @@ -2369,6 +2385,12 @@ public void readFromVectoredRange(ParquetFileRange currRange, ChunkListBuilder b currRange, timeoutSeconds); buffer = FutureIO.awaitFuture(currRange.getDataReadFuture(), timeoutSeconds, TimeUnit.SECONDS); + // Register the buffer for release as soon as it is acquired, before running any code that + // could throw (the metrics callback below is user-supplied). Otherwise an exception here + // would leak this buffer, since the row group has not yet taken ownership of the releaser. + // Requires fs.file.checksum.verify=false so the returned buffer is the allocator buffer + // rather than a sliced subset (see Hadoop's fs.file.checksum.verify docs). + builder.addBuffersToRelease(Collections.singletonList(buffer)); setReadMetrics(readStart, currRange.getLength()); // report in a counter the data we just scanned BenchmarkCounter.incrementBytesRead(currRange.getLength()); diff --git a/parquet-hadoop/src/test/resources/core-site.xml b/parquet-hadoop/src/test/resources/core-site.xml new file mode 100644 index 0000000000..a6344b87c9 --- /dev/null +++ b/parquet-hadoop/src/test/resources/core-site.xml @@ -0,0 +1,33 @@ + + + + + + fs.file.checksum.verify + false + + Disable checksum verification on the local file system used by tests. + Hadoop's ChecksumFileSystem.readVectored allocates checksum buffers via + the caller-supplied ByteBufferAllocator without releasing them; the + leaked buffers trip TrackingByteBufferAllocator leak detection in tests. + Turning off checksum verification skips the checksum read path entirely + and avoids the leak. + + + diff --git a/pom.xml b/pom.xml index ec53d3d721..f184c263c3 100644 --- a/pom.xml +++ b/pom.xml @@ -83,7 +83,7 @@ 3.8.0 shaded.parquet - 3.3.0 + 3.4.2 1.18.0 thrift ${thrift.executable}