Repository navigation
GH-1234: Release serialization buffers when compression fails - #1290
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| @ParameterizedTest | ||
| @ValueSource(longs = {0, 16}) | ||
| void compressionAllocationFailureReleasesBuffers(long availableBytes) { | ||
| try (BufferAllocator allocator = new RootAllocator(); | ||
| IntVector vector = new IntVector("values", allocator); | ||
| VectorSchemaRoot root = VectorSchemaRoot.of(vector)) { | ||
| vector.allocateNew(1); | ||
| vector.set(0, 42); | ||
| root.setRowCount(1); | ||
| long allocatedBefore = allocator.getAllocatedMemory(); | ||
| int referencesBefore = vector.getDataBuffer().getReferenceManager().getRefCount(); | ||
| allocator.setLimit(allocatedBefore + availableBytes); | ||
| VectorUnloader unloader = new VectorUnloader(root, true, new CopyCodec(), true); | ||
|
|
||
| assertThrows(OutOfMemoryException.class, unloader::getRecordBatch); | ||
| assertEquals(allocatedBefore, allocator.getAllocatedMemory()); | ||
| assertEquals(referencesBefore, vector.getDataBuffer().getReferenceManager().getRefCount()); | ||
| assertEquals(42, vector.get(0)); |
| @Override | ||
| public CompressionUtil.CodecType getCodecType() { | ||
| return CompressionUtil.CodecType.LZ4_FRAME; | ||
| } |
jbonofre
left a comment
There was a problem hiding this comment.
Thanks for the fix and good test coverage @pj-workspace!
The separation between VectorUnloader and AbstractCompressionCodec is clean and strictly adheres to the CompressionCodec contract:
- using
try (uncompressedBuffer)inAbstractCompressionCodec.compress()ensures the input buffer reference on all exit paths (empty check, compression failure, packaging fallback) without double-freeing - the rollback cleanup via
AutoCloseable.close(e, buffers)inVectorUnloader.getRecordBatch()reliably frees collected buffers and preserves the root exception - the regression test suite is great.
(nit and non blocker: we could apply similar try-with-resource semantics to AbstractCompressionCodec.decompress() and align StructVectorUnloader in the C module)
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
dc61ee7 to
a5bf863
Compare
…tch fails ### What changes were proposed in this pull request? This PR fixes an Arrow memory leak when compressing a record batch fails, as a workaround until Arrow Java 20.0.0. It adds `ArrowCompressionUtils.createVectorUnloader` and uses it in all nine places that create a `VectorUnloader` with a codec: `ArrowConverters.ArrowBatchIterator`, `ArrowCachedBatchSerializer` (3), `PythonArrowInput` (2), `ColumnarArrowPythonInput` (2) and `CoGroupedArrowPythonRunner`. With a compressing codec, it returns a `ReleasingVectorUnloader`, which extends Arrow's `VectorUnloader` and wraps the codec in a `TrackingCompressionCodec`: - `TrackingCompressionCodec` keeps the buffer that the batch holds for each `compress` call: the input until the call returns, and the output after that. - When `getRecordBatch` throws, `ReleasingVectorUnloader` closes these buffers with `AutoCloseables.close`, which adds a failure of `close()` to the original exception as a suppressed exception. When it returns, the batch owns the buffers, so the list is only cleared. With `NoCompressionCodec`, which cannot fail, `createVectorUnloader` returns Arrow's own `VectorUnloader`, so the default configuration is unchanged. Arrow Java 20.0.0 fixes this in `VectorUnloader` and `AbstractCompressionCodec` (apache/arrow-java#1290). `ReleasingVectorUnloader` must be removed with that upgrade, because it would release the buffers twice. The new test then fails with `RefCnt has gone negative`, which I checked by running it with the `VectorUnloader` and `AbstractCompressionCodec` changes of apache/arrow-java#1290 on the test classpath. Alternatives that were not taken: - Wrap only the codec in `createCompressionCodec`: a codec does not know where a batch ends, so it cannot tell the buffers of the failed batch from those of the batches already returned. - Copy `VectorUnloader.getRecordBatch` and `appendNodes` into Spark: it brings Arrow's traversal and buffer layout checks into Spark. ### Why are the changes needed? Arrow Java up to 19.0.0 does not release buffers when `CompressionCodec.compress` throws inside `VectorUnloader.getRecordBatch` (apache/arrow-java#1234): - `appendNodes` retains each buffer before passing it to `compress`, and `AbstractCompressionCodec.compress` closes its input only on success. So the buffer being compressed keeps the extra reference. - The buffers already compressed for the batch are only in a local list of `getRecordBatch`, which nothing releases. Closing the vectors and the allocator cannot release these buffers, so they stay allocated until the JVM exits. The realistic trigger is direct memory running out, because compression allocates an additional direct buffer per input buffer. Arrow's Netty allocator turns direct memory exhaustion into `org.apache.arrow.memory.OutOfMemoryException`, which is not fatal to the executor. So the executor keeps running with less direct memory for the next tasks. The task still fails with the original exception, because `TaskContext.runTaskWithListeners` only adds a failure of a completion listener as a suppressed exception. Closing the allocator reports the leak, as in this example with a capped allocator. ``` org.apache.arrow.memory.OutOfMemoryException: Unable to allocate buffer of size 256 (rounded from 196) due to memory limit. Current allocation: 65536 at org.apache.arrow.vector.compression.AbstractCompressionCodec.compress(AbstractCompressionCodec.java:41) at org.apache.arrow.vector.VectorUnloader.appendNodes(VectorUnloader.java:131) ... Suppressed: org.apache.spark.util.TaskCompletionListenerException: Memory was leaked by query. Memory leaked: (16384) ``` On the zero-copy paths (`ArrowCachedBatchSerializer.convertArrowBatchZeroCopy` and `ColumnarArrowPythonInput.writeArrowDirect`), the buffers belong to the allocator of the upstream vectors, so the leak is reported by the owner of those vectors rather than by the code that failed. This only happens when `spark.sql.execution.arrow.compression.codec` is `zstd` or `lz4`; the default is `none`. By code reading, the affected paths have existed since Apache Spark 4.1.0 for `ArrowConverters`, `PythonArrowInput` and `CoGroupedArrowPythonRunner` (SPARK-54134, SPARK-54226), since Apache Spark 4.2.0 for `ColumnarArrowPythonInput` (SPARK-56350), and since Apache Spark 4.3.0 for `ArrowCachedBatchSerializer` (SPARK-57268). ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs with the newly added test case in `ArrowCompressionUtilsSuite`. Its codec fails while the second batch compresses its second buffer. The test checks that no buffer is released twice, that the first batch keeps its buffers, and that closing the allocator finds no leak. I also ran the affected paths in tasks with a temporary suite, which is not part of this PR. It caps `ArrowUtils.rootAllocator` when the input rows run out, so that the first allocation inside `compress` fails, and then measures the memory left behind. | Path | Before | After | |---|---|---| | `ArrowBatchIterator` | 16384 bytes | 0 bytes | | `InternalRowToArrowCachedBatchIterator` | 16384 bytes | 0 bytes | | `ColumnarBatchToArrowCachedBatchIterator` zero-copy, in the upstream allocator | 8192 bytes | 0 bytes | ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59991 from dongjoon-hyun/SPARK-60776. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…tch fails ### What changes were proposed in this pull request? This PR fixes an Arrow memory leak when compressing a record batch fails, as a workaround until Arrow Java 20.0.0. It adds `ArrowCompressionUtils.createVectorUnloader` and uses it in all nine places that create a `VectorUnloader` with a codec: `ArrowConverters.ArrowBatchIterator`, `ArrowCachedBatchSerializer` (3), `PythonArrowInput` (2), `ColumnarArrowPythonInput` (2) and `CoGroupedArrowPythonRunner`. With a compressing codec, it returns a `ReleasingVectorUnloader`, which extends Arrow's `VectorUnloader` and wraps the codec in a `TrackingCompressionCodec`: - `TrackingCompressionCodec` keeps the buffer that the batch holds for each `compress` call: the input until the call returns, and the output after that. - When `getRecordBatch` throws, `ReleasingVectorUnloader` closes these buffers with `AutoCloseables.close`, which adds a failure of `close()` to the original exception as a suppressed exception. When it returns, the batch owns the buffers, so the list is only cleared. With `NoCompressionCodec`, which cannot fail, `createVectorUnloader` returns Arrow's own `VectorUnloader`, so the default configuration is unchanged. Arrow Java 20.0.0 fixes this in `VectorUnloader` and `AbstractCompressionCodec` (apache/arrow-java#1290). `ReleasingVectorUnloader` must be removed with that upgrade, because it would release the buffers twice. The new test then fails with `RefCnt has gone negative`, which I checked by running it with the `VectorUnloader` and `AbstractCompressionCodec` changes of apache/arrow-java#1290 on the test classpath. Alternatives that were not taken: - Wrap only the codec in `createCompressionCodec`: a codec does not know where a batch ends, so it cannot tell the buffers of the failed batch from those of the batches already returned. - Copy `VectorUnloader.getRecordBatch` and `appendNodes` into Spark: it brings Arrow's traversal and buffer layout checks into Spark. ### Why are the changes needed? Arrow Java up to 19.0.0 does not release buffers when `CompressionCodec.compress` throws inside `VectorUnloader.getRecordBatch` (apache/arrow-java#1234): - `appendNodes` retains each buffer before passing it to `compress`, and `AbstractCompressionCodec.compress` closes its input only on success. So the buffer being compressed keeps the extra reference. - The buffers already compressed for the batch are only in a local list of `getRecordBatch`, which nothing releases. Closing the vectors and the allocator cannot release these buffers, so they stay allocated until the JVM exits. The realistic trigger is direct memory running out, because compression allocates an additional direct buffer per input buffer. Arrow's Netty allocator turns direct memory exhaustion into `org.apache.arrow.memory.OutOfMemoryException`, which is not fatal to the executor. So the executor keeps running with less direct memory for the next tasks. The task still fails with the original exception, because `TaskContext.runTaskWithListeners` only adds a failure of a completion listener as a suppressed exception. Closing the allocator reports the leak, as in this example with a capped allocator. ``` org.apache.arrow.memory.OutOfMemoryException: Unable to allocate buffer of size 256 (rounded from 196) due to memory limit. Current allocation: 65536 at org.apache.arrow.vector.compression.AbstractCompressionCodec.compress(AbstractCompressionCodec.java:41) at org.apache.arrow.vector.VectorUnloader.appendNodes(VectorUnloader.java:131) ... Suppressed: org.apache.spark.util.TaskCompletionListenerException: Memory was leaked by query. Memory leaked: (16384) ``` On the zero-copy paths (`ArrowCachedBatchSerializer.convertArrowBatchZeroCopy` and `ColumnarArrowPythonInput.writeArrowDirect`), the buffers belong to the allocator of the upstream vectors, so the leak is reported by the owner of those vectors rather than by the code that failed. This only happens when `spark.sql.execution.arrow.compression.codec` is `zstd` or `lz4`; the default is `none`. By code reading, the affected paths have existed since Apache Spark 4.1.0 for `ArrowConverters`, `PythonArrowInput` and `CoGroupedArrowPythonRunner` (SPARK-54134, SPARK-54226), since Apache Spark 4.2.0 for `ColumnarArrowPythonInput` (SPARK-56350), and since Apache Spark 4.3.0 for `ArrowCachedBatchSerializer` (SPARK-57268). ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs with the newly added test case in `ArrowCompressionUtilsSuite`. Its codec fails while the second batch compresses its second buffer. The test checks that no buffer is released twice, that the first batch keeps its buffers, and that closing the allocator finds no leak. I also ran the affected paths in tasks with a temporary suite, which is not part of this PR. It caps `ArrowUtils.rootAllocator` when the input rows run out, so that the first allocation inside `compress` fails, and then measures the memory left behind. | Path | Before | After | |---|---|---| | `ArrowBatchIterator` | 16384 bytes | 0 bytes | | `InternalRowToArrowCachedBatchIterator` | 16384 bytes | 0 bytes | | `ColumnarBatchToArrowCachedBatchIterator` zero-copy, in the upstream allocator | 8192 bytes | 0 bytes | ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59991 from dongjoon-hyun/SPARK-60776. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit e438d0c) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What's Changed
When record batch serialization fails after retaining or compressing buffers,
VectorUnloadercan leave those buffers alive even after the source vectors are closed. Compression allocation failures can also leave an extra reference to the input buffer inAbstractCompressionCodec.Release collected batch buffers if traversal or batch construction throws, while preserving the original exception. Use try-with-resources in
AbstractCompressionCodec.compressso the codec releases the input it owns on both success and failure. Cleanup remains in the component that owns each buffer: the unloader does not release an input already handed to a custom codec.Regression coverage checks allocator exhaustion with zero allocation headroom, runtime exceptions and errors injected on the second compression call, an empty input buffer, and a malformed later vector. The compression-only test stub is explicitly named
SimulatedLz4Codec; it exercises allocation and ownership, not LZ4 round trips. The tests assert memory usage and reference counts return to their previous values and that the source data remains readable. Additional tests exercise allocation failure with the real LZ4 and ZSTD codecs.The memory-leak report and initial outer-cleanup proposal are from #1234; this change also handles the codec's input ownership.
Validation
git diff --checkpassed.mvn -B -ntp -pl compression -am test -Dtest=TestVectorUnloadLoad,TestVectorUnloaderFailure,TestCompressionCodec,TestArrowReaderWriterWithCompression,TestCompressionCodecServiceProvider -Dsurefire.failIfNoSpecifiedTests=falseCloses #1234.
AI assistance: OpenAI Codex was used for implementation and local validation.