andygrove commented on PR #5537: URL: https://github.com/apache/datafusion-comet/pull/5537#issuecomment-5464233991
`canAggregateBeConverted` documents itself as a conservative check and every other branch returns a bare `false`, but the new FIRST/LAST branch calls `withFallbackReason` on the aggregate. That makes a predicate which runs over the pre-conversion plan mutate it. As far as I can tell the tag cannot change any conversion decision. The only readers of `hasFallbackReason` in this file are `reportUnexplainedFallback` and the generic-message path in the `case op =>` fallthrough, and both use it to decide whether to add "is not supported" or throw under `COMET_STRICT_FALLBACK_REASONS`, never to refuse a conversion. And when the same node reaches `CometBaseAggregate.doConvert`, the string written there at `operators.scala:1785` is character for character the one written here, so it dedupes into the same `FALLBACK_REASONS` set and the explain output is identical either way. Would dropping the `withFallbackReason` call and just returning `false` work here? The one thing it would give up is pre-satisfying the strict-reason check for that node, which `doConvert` already covers on its own. -- 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]
