andygrove commented on PR #5809: URL: https://github.com/apache/datafusion-comet/pull/5809#issuecomment-5656824227
I opened #5909 for the same problem before finding this PR, sorry for the overlap. It is now a draft, and this PR has priority since it came first. Posting the comparison here because the two take different routes to the same cache and the numbers suggest they are complementary rather than competing. **Where the cost is.** Both PRs agree the schema parse is a fixed cost per block. On my machine (Apple Silicon, codec None, the same `shuffle_reader` bench shapes) `parse_schema_only` is 0.8 µs at 5 columns and 6 µs at 50 columns, which is about half of a 64-row block's decode and under 10 percent of an 8192-row block's. So the parse matters most for small blocks, and the body zero-fill plus copy that this PR removes matters most for large ones. **What #5909 does differently.** It keeps the streaming `Read`-based path and replaces `StreamReader` with a message-level loop over the same framing (schema, dictionary batches, one record batch, end of stream, plus the existing trailing-data and single-batch checks). The decoder keeps the raw bytes of the last schema message next to the parsed `SchemaRef`; each block's schema message is compared with `==` on the bytes and reused on a match, parsed and cached otherwise. It does not materialize the block, so it never pays for a block the old path did not pay for. The body is still read into a `MutableBuffer` as before, so it does not get the large-block win this PR measures. Relative change of a decoder held across blocks versus a fresh decoder per block, same branch, same machine: | shape | #5909 cached vs fresh | this PR (from the description) | | --- | --- | --- | | 5 col x 64 row | -54% | +15% | | 5 col x 512 row | -33% | +4% | | 5 col x 8192 row | -8% | -29% | | 50 col x 64 row | -54% | -2% | | 50 col x 512 row | -39% | -9% | | 50 col x 8192 row | -7% | -30% | The fresh-decoder numbers in #5909 are unchanged from `main`, so the message loop is free on a cache miss. **Suggestion.** Both PRs key the cache on the raw schema message bytes, so the cache itself is the same idea. The difference is what happens to the body: this PR materializes the block and decodes in place, #5909 streams it. The two stack naturally: the streaming loop for blocks under a size threshold, where materialization does not pay for itself, and your in-place decode above it. That would give the small-block gain without the regression you flagged and keep the large-block gain, which is the size gate you offered in the description. Happy to close #5909 once that lands, or to rebase #5909 on top of this if you would rather keep the in-place decode as the base. Whichever you prefer. One more thing worth a test if it is not already covered: the JVM columnar shuffle writes dictionary-encoded strings, so blocks on that path carry a dictionary batch before the record batch, and the dictionary must be scoped to its own block rather than to the cached schema. -- 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]
