sunchao commented on PR #7190: URL: https://github.com/apache/datafusion-comet/pull/7190#issuecomment-6429392565
Thanks @andygrove and @viirya. The updates in 10de354986 and 8650e826 address the implementation and test requests. I have also replied to each inline comment. d92d50e606 only clarifies two source comments. For Andy's three requests: - Removed the scratch reservation and `with_can_spill` machinery. The PR description includes tight fair-unified-pool controls with a real JVM sorter in the same task. Across 215 paired runs, answers and JVM/native spill measurements matched. The admission check now runs at first native execution and covers consumers already holding memory then. - Parameterized the native nullable/sliced-array test over all four signed integer widths. The SQL test now uses BIGINT, including a value above 32 bits and an unmatched low-32-bit alias. - Added `(-1, 20)` only in the final one-row right batch. The SQL test verifies that it matches left id 7 after successful reduction and on the bypass path, while preserving duplicate left rows. For Viirya's broader questions: - The PR description now includes whole-query Spark timings using the official optimized native library for 8650e826. With 33.6 million right rows containing 1,024 repeated IDs, median time was 1.382 s without reduction and 0.951 s with it. The unique-key control was 2.027 s versus 2.030 s. These are eight alternating measured pairs after warm-up, on local warm Parquet with fresh shuffles, and exclude network shuffle effects. - A partial distinct below the exchange could reduce shuffle traffic and catch cross-batch duplicates. It would also add map-side aggregation state and its memory/spill behavior. This change stays focused on sorting decoded shuffle batches, so shuffle traffic is unchanged. - The primitive cursor is not structurally immune to many small batches. The reducer now packs tiny outputs before sorting, with a unit test covering packing, bypass ordering, and EOF. The published native control with 262,144 constant batches and a 14 MiB fair pool reduced peak live Rust allocations from 236.8 MB to 3.3 MB and spills from 1,042 to 2, preserving results. These are local stress controls, not a newly committed spilling regression test. The reservation/refusal machinery, stale ownership wording, fixed user-guide thresholds, default assertions, and benchmark verification duplication have been removed. The prefix gate and shuffle scope remain deliberate choices, with their rationale and the prefix-specific measurements in the inline replies. I am leaving those two discussions open for your assessment. -- 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]
