peterxcli commented on PR #5809: URL: https://github.com/apache/datafusion-comet/pull/5809#issuecomment-5676938081
> Replying to https://github.com/apache/datafusion-comet/pull/5809#pullrequestreview-5191026120 Thanks for the careful read. Pushed a5290ca76, which restructures the decoder rather than patching the probe, and reworked the description to match. Point by point: **CI.** The red `rust-test` is `execution::operators::iceberg_write::tests::cancelling_abort_keeps_the_guard_armed` panicking on "expected the deletes to yield so the abort can be cancelled mid-flight". It is scheduling-sensitive and nothing in this PR touches `iceberg_write`; it went red on the merge commit because that is the run it happened to land on. Your #5919 is the fix. The new push re-runs the job. **Lz4Frame in the matrix.** Added, alongside `None`, and re-run on the same idle 16-core host with base/new alternated. Table in the description. Under LZ4 the cached decode is 30 to 40 percent faster at 64 rows, 20 to 24 percent at 512, and 2 to 5 percent at 8192 rows where decompression dominates. The miss path is flat to slightly faster on every LZ4 shape, so the cache costs nothing on the default codec. The `read_to_end` growth you described went with the materialization: compressed bodies are now read straight into an exactly sized buffer, as before this PR. **Tests that would survive removing the fast path.** There is no separate fast path any more, but the equivalent gap is covered: under `cfg(test)` the cache counts hits and misses, `reset_schema_cache` clears both, and every cache test now asserts the exact counts it expects, so a decode that silently re-parsed the schema would fail `warm_decodes_hit_the_cache_and_match_the_cold_one`. The dictionary test asserts a hit rather than a declined probe, because dictionary blocks now decode from the cache too (below), and it checks that each block's record batch is decoded against that block's dictionary and not the previous block's. **Four `root_as_message` calls per block.** On a hit the schema message is now not parsed at all: the reader locates it by its length prefix, memcmps it against the cache, and moves on. The record batch message is parsed once, dictionary messages once each. `IpcMessage` is gone; the loop keeps the parsed `Message` for the duration of the message it describes. This is what turned the small-block regression into a gain, so no body-size threshold. **Dictionary blocks.** They take the same loop: dictionary batches decode with `read_dictionary_impl` into a map scoped to the block, then the record batch decodes against it. No failed probe and no materialization. The benchmark gained `5col_64row_dict` and `5col_8192row_dict` arms on both codecs. Cached dictionary blocks are 38 percent faster uncompressed and 28 percent under LZ4 at 64 rows, 39 and 4.5 percent at 8192 rows. On a miss the 64-row shapes are within noise, so no probe cost remains. **Memory.** The whole-block `read_to_end` is gone. Compressed bodies use the same `MutableBuffer::from_len_zeroed(bodyLength)` `StreamReader` used; uncompressed bodies are an exactly sized copy of the body slice without the zero fill. `decoded_arrays_report_the_same_memory_size_as_stream_reader` asserts `get_array_memory_size` equals a plain `StreamReader` decode on every codec, cold and warm, so what DataFusion reserves for a shuffle-read batch is unchanged. I also bounded the per-thread metadata scratch: a corrupt length that grows it past 1 MiB releases it after the block. I have not measured executor RSS across a Spark shuffle read; with per-body allocations identical to the previous path I do not expect it to show anything, but I can run one if you would like the number. -- 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]
