mohitgurav20 commented on PR #25716: URL: https://github.com/apache/datafusion/pull/25716#issuecomment-6084742359
Thanks for the thorough review and suggestions! @kosiew I've pushed a commit that addresses all four of your inline suggestions: Clarified in the docs that deduplication uses the (pointer, length) pair. Updated the RecordBatch::try_new comment to explain that the original batch's schema, column count, lengths, types, and null validity are all preserved. Clarified that the fast path avoids rewriting the views, rather than claiming zero allocations. Strengthened the reservation tests by adding assert_eq!(reservation.size(), get_record_batch_memory_size(&batch)) to all three deduplication tests to ensure the memory pool accurately reflects the shrunk footprint. Regarding the benchmark investigation for Q64 (pushdown_filters = true): I investigated this regression and found the culprit. The deduplication logic was using std::collections::HashMap which uses the notoriously slow SipHash. Because pushdown_filters=true creates a high volume of heavily-filtered, fragmented batches, this hashing overhead was causing the regression. I have swapped it to use hashbrown::HashMap (which uses the much faster ahash algorithm standard in DataFusion) and this should resolve the slowdown! All tests are passing. Let me know if you need anything else! -- 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]
