andygrove commented on PR #5607: URL: https://github.com/apache/datafusion-comet/pull/5607#issuecomment-5592015623
Thanks for working through this, and for measuring rather than assuming. The thing that stands out at `4feac844` is that the description no longer matches the code. It still says the three serdes mix in `CodegenDispatchFallback`, that six docs rows moved from Native to Hybrid, and that the binary fixtures assert parity instead of `expect_fallback`. After the revert none of that is true, and the fixtures assert the opposite. It also still says `Closes #5584`, so merging as-is would auto-close an issue that nobody has implemented. Could you rewrite the description and unlink the issue, and post the measurement on #5584 instead, so that the next person to pick it up finds the numbers? I checked out the head and planned all five of the benchmark's query shapes with `spark.comet.exec.scalaUDF.codegen.enabled` on and off. They are identical in every shape apart from the exprId suffixes: `Project` over `CometColumnarToRow` over `CometNativeScan`, and a plain Spark `HashAggregate` for the aggregate case. That is what I would expect once the mixin is gone, since nothing else in those queries is dispatcher-eligible. So the third arm is measuring the same plan as the second in all 21 scenarios, and the digest check cannot notice, because equal plans give equal digests. `CometCodegenDispatchBenchmark` has a guard for exactly this at line 341 that prints "the two Comet cases below are measuring the same plan". Worth knowing too that `benchmarks/micro/run.py` discovers benchmarks from that directory automatically, so this would run on every micro-benchmark run rather than only when someone asks for it. That leads to the bigger problem. The comments in `strings.scala` cite this benchmark as the evidence for staying on Spark fallback, but the benchmark cannot produce that evidence any more, because the path it compared against was deleted in the same PR. The numbers only exist at `0f579eafb`, which goes away on squash. Your last run is also a useful accident. Since both Comet arms execute the same plan, the spread between them is this harness's noise floor. At 64 B that is under one percent. At 1 KB it runs from -15.8% to +20.1% (`length` 1065 vs 1279, combined roots 1199 vs 1009). Against that, the +25.6% at 64 B that motivated the revert looks real, but the +27.6% at 1 KB is inside the noise, so I would not lean on the payload-scaling claim. Where does that leave things? The `length.sql` binary block is worth keeping on its own. It fills a gap that the other two fixtures already covered, and it passes. Beyond that I would drop `CometBinaryLengthBenchmark` rather than try to repair it. If you still want a binary length benchmark, three entries in `CometStringExpressionBenchmark`'s existing `StringExprConfig` table plus one binary corpus column gets the Spark versus Comet comparison in about four lines, and it inherits the "is the Comet plan fully native" warning from `runExprBenchmark`. Would that work for you? -- 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]
