andygrove commented on PR #4591: URL: https://github.com/apache/datafusion-comet/pull/4591#issuecomment-5080733550
Thanks for the continued iteration @pchintar. I merged latest `main` in and took another pass. The fixes from the last round all hold up: the `DefaultCachedBatch` read path with its regression test, the reordered-projection guard, the `count(*)` and NaN coverage, and the benchmark numbers in the description. A few new things from this round, only the first of which I'd like a reply on before merge. ### Asymmetric fallback when the feature is on but the serializer is foreign In `CometExecRule`, the three reachable outcomes for an `InMemoryTableScanExec` are: - feature off + Comet serializer: `withFallbackReason`, then tries `CometSparkToColumnarExec` - feature off + foreign serializer: falls through to the general `shouldApplySparkToColumnar` case - feature on + foreign serializer: `withFallbackReason`, then returns the bare `scan` That last branch returns a plain Spark row-based scan and skips the `CometSparkToColumnarExec` conversion that both feature-off branches would apply. So enabling the cache feature while a custom `spark.sql.cache.serializer` is installed is actually worse for that scan than leaving the feature off. Should this branch fall through to `shouldApplySparkToColumnar` instead of bailing entirely? Is the current behavior intentional? ### Empty-projection width mismatch For `SELECT count(*)`, `CometInMemoryTableScanExec.output` stays empty while `convert` and `createExec` fall back to `relation.output`, so the native scan is built and executed at full width. The new `count(*)` test passes, so this works for the aggregate-on-top shape. Could you add a one-line comment on `output` explaining that the declared output can be narrower than the emitted batch width in the empty-projection case, and why that is safe? Right now the reasoning is split across two methods. ### Nit: redundant import `import org.apache.spark.sql.comet.CometInMemoryTableScanExec` sits right below the existing `org.apache.spark.sql.comet._` wildcard, so it is redundant. Nothing here blocks landing. The functional path is well tested and the perf follow-ups are tracked in #4781. The routing question in the first point is the only one I'd like to resolve first. -- 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]
