comphead commented on PR #7345: URL: https://github.com/apache/datafusion-comet/pull/7345#issuecomment-6533128456
Thanks @andygrove, both are addressed in 0d514bb0cd. **1.** `merge_batch` now gets its union from `union_mut`, which swaps a `Sketch` for a `Union` and then calls itself, so it can only return through the `Union` arm and the `unreachable!` is gone. The `update_batch` fallback already holds the union from its own match, so it doesn't need it. **2.** No test compared `hll_sketch_agg` with Spark. The `CometExpressionSuite` tests check the true count within 5%. The new `expressions/aggregate/hll_sketch_agg.sql` compares with Spark over four files, each one partial per group that the Final aggregation merges. It expects exact results for every input type with NULLs and bounds, for groups whose union stays a coupon set (lgConfigK 21), and for groups merging partials that already hold the register array (lgConfigK 12, grouped and global). At lgConfigK 18 coupon-set partials take the union to the register array during the third merge. From there the estimate depends on merge order in Spark as well, so that query compares within 1%. I haven't run the file against Spark locally. Its expectations come from replaying both sides on the same data: datasketches-java 6.1.1 and 6.2.0 (Spark 4.0 and 4.1, which agree everywhere) against `HllSketchAgg` in a Final `AggregateExec`. For the fixture's four partials I tried all 24 merge orders. | Shape | This PR vs Spark | Before vs Spark | |---|---|---| | lgConfigK 21, coupon sets | equal | equal | | lgConfigK 12, dense partials | equal | equal | | lgConfigK 12, 300 list partials turning dense | equal | 6 of 1,500 | | lgConfigK 18, coupon sets turning dense | up to 54 of 40,000 | up to 70 | | Issue probe, 400 partials of 200 values | up to 5 of 80,000 | up to 156 | On the lgConfigK 18 shape Spark's own estimate moves by up to 103 between merge orders. What is left there is coupon order. The estimate depends on the order the coupons reach the union, and the two engines read a coupon-set partial back in different orders. With list partials, whose order both keep, the results are equal. One difference predates this PR. When a group's only partial is already dense, `datasketches` 0.3.0 marks the new union out of order, so Comet uses the composite estimate where Spark keeps HIP, 684 against 695 for 700 values at lgConfigK 12. Taking a group's first partial as is would match Spark there, since that partial read back gives 695. I'd leave it for a follow-up. -- 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]
