viirya commented on PR #6071: URL: https://github.com/apache/datafusion-comet/pull/6071#issuecomment-5765361206
Review follow-up: - The JNI signature assertion is fixed in c2e48d2a4225e38436b5669e1af881b468d09838. The full `CometNativeShuffleSuite` passed locally on Spark 4.1 (57 tests). - I completed matched off / decoded-cache-only / sharing measurements. All 144 timed Spark samples matched a Comet-disabled reference. The table below compares sharing with **decoded-cache-only**, so protobuf caching is enabled on both sides. | Workload | Partitions | Cache-only elapsed | Sharing elapsed | Elapsed change | Task CPU change | |---|---:|---:|---:|---:|---:| | Ordinary projection/filter, 16 columns | 1,024 | 506 ms | 494 ms | -2.4% | -38.6% | | Ordinary projection/filter, 64 columns | 1,024 | 1,408 ms | 1,215 ms | -13.7% | -55.6% | | Long-task projection/filter, 16 columns | 1,024 | 731 ms | 521 ms | -28.7% | -42.8% | | Long-task projection/filter, 16 columns | 8,192 | 5,908 ms | 8,005 ms | +35.5% | +21.6% | These are pooled medians from two sequential JVMs on M4 Max, Spark 4.1.3, `local[4]`, 32 input rows per partition. The ordinary workload uses Range input, batch size 8,192, four warmups and eight measured rounds per mode/JVM. The long-task workload uses RDD row input and batch size 16. Partition 0 pauses after the first batch until the other tasks complete. It uses two warmups and four measured rounds per mode/JVM, with assertions checking the gate and task completion order. The ordinary and long-task workloads should not be compared directly with each other. Mode order rotates between rounds. The separate release-mode native probe also completed three repetitions per configuration with real operators and a held partition-0 stream. Cumulative snapshot/filter time in sharing mode was 51.3 ms, 3.90 s and 133.6 s at 1,024, 8,192 and 32,768 partitions. Cache-only was 0.142 ms, 1.11 ms and 4.84 ms. These are mechanism timings over the completed short partitions, not Spark query timings. The metrics finding remains open. The long-task Spark result confirms a regression, while the native probe establishes the snapshot scaling mechanism. The short-task gains do not justify merging the current reporting path. A bound on the number of partitions served by each physical tree is a possible approach without changing DataFusion, but its reuse/construction tradeoff still needs validation. I have not applied that change or treated this issue as resolved. Per-key registry build slots address a separate lock-contention issue. -- 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]
