peterxcli commented on PR #5809: URL: https://github.com/apache/datafusion-comet/pull/5809#issuecomment-5677467957
> Replying to https://github.com/apache/datafusion-comet/pull/5809#pullrequestreview-5204781228 Thanks for the thorough pass. That review is against 0f61f3510; a5290ca76 (pushed shortly before it landed) replaced the design it describes, and fe93ff220 adds what was still missing. Mapping the findings to the current head: **P2, whole-block materialization and the dictionary fallback.** Gone. The reader walks each block message by message the way `StreamReader` does; there is no full-block `Buffer`, no growing `Vec`, and no fallback decoder. Dictionary batches decode in the same loop with `read_dictionary_impl`, scoped to their block, from the cached schema. Cold misses parse the schema exactly as `StreamReader` did and read the same exactly sized bodies. **Schema and record-batch metadata verified twice.** Gone. A hit does not parse the schema message at all (length prefix, memcmp, move on); every other message is parsed once and the parsed `Message` is carried through the decode. **Spare `Vec` capacity under live arrays.** Gone with the materialization. Compressed bodies use `MutableBuffer::from_len_zeroed(bodyLength)` as before; uncompressed bodies are an exactly sized copy. `decoded_arrays_report_the_same_memory_size_as_stream_reader` asserts `get_array_memory_size` equals a plain `StreamReader` decode on every codec. **Tests that prove the optimized path runs.** Under `cfg(test)` the cache counts hits and misses and `reset_schema_cache` clears both, so every cache test states its cold and warm phases and asserts exact counts: cold reset then hit, dictionary blocks served from the cache with per-block dictionaries, alternating schemas staying resident, eviction past the four-entry capacity, and (fe93ff220) the corrupt-offsets block still failing validation when the schema is served from the cache by an earlier valid block. **Allocation and copy volume, peak live memory, retained capacity.** fe93ff220 shares the RSS tests' allocation observer with the reader tests. `warm_decode_allocates_no_more_than_stream_reader` runs the old `StreamReader` path and the cached decode on the same bytes and asserts the new one never allocates more, in count, bytes, or peak: One warm decode, counted with the crate's test allocation observer (Rust allocations only; zstd's C-side workspace is outside it, which is why ZSTD reads like NONE). Old is the `StreamReader` path this PR replaced, run in the same test on the same bytes: | block | codec | old: allocations / bytes / peak live | this PR: allocations / bytes / peak live | | --- | --- | --- | --- | | 3 rows, 3 columns | none | 22 / 2,451 / 1,675 | 12 / 1,248 / 920 | | 3 rows, 3 columns | lz4 | 24 / 133,523 / 132,747 | 14 / 132,320 / 131,992 | | 3 rows, 3 columns | zstd | 22 / 2,451 / 1,675 | 12 / 1,248 / 920 | | 3 rows, 3 columns | snappy | 24 / 144,477 / 143,701 | 14 / 143,274 / 142,946 | | 8192 rows, 2 columns | none | 18 / 150,402 / 149,554 | 9 / 149,032 / 148,816 | | 8192 rows, 2 columns | lz4 | 20 / 674,690 / 673,842 | 11 / 673,320 / 673,104 | | 8192 rows, 2 columns | zstd | 18 / 150,402 / 149,554 | 9 / 149,032 / 148,816 | | 8192 rows, 2 columns | snappy | 20 / 292,428 / 291,580 | 11 / 291,058 / 290,842 | The difference is the schema parse and the metadata buffer, about ten allocations and 1.2 KiB per block; the body allocation is identical. On LZ4 and Snappy the decompressors' own working buffers dominate both columns. **Benchmark.** Re-run on an idle 16-core x86_64 host against `main` at 481aefea9, base and head alternated over two rounds, same benchmark source on both sides, commit identity in the description. `None` and `Lz4Frame`, 5 and 50 columns by 64, 512 and 8192 rows, plain and dictionary strings, cached and cache-cleared arms, and the untouched `parse_schema_only` control (within 3%). No shape regresses; the tables are in the description. fe93ff220 adds `decode_block_validated` for the remote entry point: Validated decode (`decode_block_validated`, the remote entry point, schema from the cache): | shape | codec | base (two rounds) | this PR (two rounds) | change | | --- | --- | --- | --- | --- | | 5 col x 64 row | lz4 | 9.71 us / 9.15 us | 6.56 us / 6.72 us | -29.6% | | 5 col x 512 row | lz4 | 25.31 us / 26.36 us | 21.18 us / 20.84 us | -18.7% | | 5 col x 8192 row | lz4 | 341.45 us / 341.73 us | 335.25 us / 335.59 us | -1.8% | | 50 col x 64 row | lz4 | 72.82 us / 72.26 us | 45.98 us / 47.36 us | -35.7% | | 50 col x 512 row | lz4 | 244.81 us / 247.56 us | 191.44 us / 198.29 us | -20.8% | | 50 col x 8192 row | lz4 | 3.76 ms / 3.83 ms | 3.73 ms / 3.68 ms | -2.3% | | 5 col x 64 row (dictionary strings) | lz4 | 14.60 us / 14.51 us | 10.04 us / 9.77 us | -31.9% | | 5 col x 8192 row (dictionary strings) | lz4 | 283.60 us / 281.65 us | 265.88 us / 269.28 us | -5.3% | | 5 col x 64 row | none | 5.09 us / 5.03 us | 2.98 us / 2.90 us | -41.9% | | 5 col x 512 row | none | 8.44 us / 8.46 us | 5.90 us / 5.93 us | -30.0% | | 5 col x 8192 row | none | 67.79 us / 66.41 us | 57.49 us / 59.16 us | -13.1% | | 50 col x 64 row | none | 48.92 us / 47.37 us | 26.70 us / 26.73 us | -44.5% | | 50 col x 512 row | none | 89.48 us / 88.57 us | 64.11 us / 66.84 us | -26.5% | | 50 col x 8192 row | none | 1.08 ms / 1.11 ms | 905.54 us / 884.60 us | -18.3% | | 5 col x 64 row (dictionary strings) | none | 9.24 us / 9.49 us | 6.65 us / 5.97 us | -32.6% | | 5 col x 8192 row (dictionary strings) | none | 40.48 us / 38.39 us | 30.70 us / 30.27 us | -22.7% | **CI.** The failure was `iceberg_write::tests::cancelling_abort_keeps_the_guard_armed`, untouched by this PR and fixed by #5919. On a5290ca76 the full `rust-test` job completed green with the new shuffle tests included. Not covered: dictionary binary columns are the same code path as dictionary strings and are not benchmarked separately, and interleaved schemas beyond the cache capacity are tested for correctness but not timed. -- 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]
