peterxcli commented on PR #5225: URL: https://github.com/apache/datafusion-comet/pull/5225#issuecomment-5195126502
@andygrove thanks for another round of review! addressed all of your review. > First, the note about approach. This PR does the opposite of what https://github.com/apache/datafusion-comet/issues/5190 proposed. It removes the serde wrapper and keeps promotion as the single owner, rather than removing the promotion wrapper. I think your choice is the better one, and it is what lets the same fix cover add, subtract, multiply, and remainder. But a reader comparing the issue to the diff will be confused without a sentence saying so. added some sentence regard the difference if #5190 (original issue) and this pr's change. > Second, the numbers. I asked for a rough before and after over a decimal-division-heavy query from tpcds-micro-benchmarks/. If you have run one, please add it. If the difference turns out to be too small to measure reliably, that is a fine answer too, but then I would rather the description lead with the idempotency and single-ownership argument and drop the framing that the duplicate scan is a meaningful cost. Either resolution works, I just do not want the perf claim sitting there unbacked. Ran benchmark with TPC-DS SF1 `store_sales` (2,879,789 rows) on an Apple M4, it shows about 6.8% improvement in time. Also added in PR description as table. > It would also help to link https://github.com/apache/datafusion-comet/issues/5248 in the description, not only in the review thread, so it stays visible from the merged commit. yes, added. > Could you add a short comment naming that dependency and linking https://github.com/apache/datafusion-comet/issues/5248, so whoever picks up that work knows this regression needs re-pointing? The promote(promote(e)) == promote(e) assertion below is independent and will still hold, so it is only the proto-shape half that is at risk. thanks for the heads-up. added. -- 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]
