Visorgood commented on PR #6110:
URL:
https://github.com/apache/datafusion-comet/pull/6110#issuecomment-6389337546
@sunchao was right: `GROUP BY bump(a)` let Spark hoist the call into the
partial aggregate, so the exchange partitioned on a plain attribute the old
probe always admitted. It is now `DISTRIBUTE BY bump(a)`, and `checkPlans` runs
the same `exprToProto` call the probe made, failing unless some exchange really
has a key it rejected. Added the shape with a Comet operator above the exchange
too.
Then I merged main and the numbers changed sign. Two runs, 8M rows, best
time:
run 1 run 2
range, Comet scan -6.0% -8.1%
range, row-based Spark scan -5.4% -7.3%
hash, Comet scan -20.5%
-18.8%
hash, row-based Spark scan -13.8% -14.9%
hash, Comet operator above, Comet scan -45.0% -38.7%
hash, Comet operator above, row-based -31.6% -33.0%
Every shape faster, both runs. The range arms had ~390ms stdev in run 1 so I
did not trust them; in run 2 they were 37ms and still faster.
We both measured on a branch whose merge base was 00a4b422f (October 9). Six
shuffle performance commits landed after it (#6824, #6805, #6861, #6943, #7065,
#7059) and `git merge-base --is-ancestor` says none was in that base, with the
last two landing after your review. The path we timed is not the one on main
today. Your method was tighter than mine, so I do not think either measurement
was wrong; the subject moved.
The Comet-operator-above shape wins most, which fits: the writer always
pulls rows through `child.execute()`, but the reader hands Arrow straight to
the operator above instead of converting back to rows.
On `branch-1.1`: my baseline arm stays inside one build, where switching
Comet shuffle off reproduces the pre-#5971 plan. With those six commits in
between, a branch-1.1 comparison would show a large win mostly not caused by
this PR. Happy to run it for the upgrade delta, but I did not want to claim
those numbers here.
@comphead: merged main, and you were right about the duplicate.
`checkPartitionAssignmentMatchesSpark` now calls `checkSparkAnswer`, so
dropping the other test loses no coverage. 134 tests passing across the four
shuffle and collation suites.
So I would rather skip the allowlist and the config – both answered a
slowdown I can no longer reproduce. Please re-measure on current main; if you
still see one, I will take the narrower route.
--
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]
