Myx778 commented on PR #4105: URL: https://github.com/apache/datafusion-comet/pull/4105#issuecomment-5032021399
Thank you very much for the detailed review and for validating the correctness against Spark's implementation, @andygrove. Your concern about the maintenance trade-off is entirely fair. I agree that keeping the pipeline native only when `COMET_SCALA_UDF_CODEGEN_ENABLED` is disabled is not, by itself, a strong enough justification, especially because the dispatcher is enabled by default and the current benchmark is roughly at parity. The intended benefits are to avoid per-task closure serialization/Janino compilation overhead and to retain native execution when the dispatcher is disabled or cannot handle the expression, but these benefits need to be demonstrated with measurements rather than assumed. I have pushed commit `e602d97f`, which syncs the branch with current `main` and addresses the smaller points you raised: - uses fallible generic string conversion and supports both `Utf8` and `LargeUtf8`; - checks collation on all string children; - removes the overlapping Scala tests in favor of the SQL file tests; - uses a banded implementation for the threshold form instead of computing the full matrix. For the native-path decision, I plan to optimize and benchmark the implementation more thoroughly, including ASCII and Unicode data, short and long strings, literal and column operands, several threshold values, and dispatcher-enabled/disabled execution. The main optimization candidates are an ASCII fast path, common-prefix/suffix trimming, reusable batch-level work buffers, and a bit-parallel path for short strings. If those changes do not produce a clear, repeatable advantage over `CometCodegenDispatch`, I agree that retaining the Spark-exact dispatcher as the default is preferable to adding a hand-written native implementation without sufficient benefit. The new CI workflows have been triggered, but they are currently waiting for an Apache repository maintainer to approve the fork workflow runs. Thank you again for the thoughtful feedback. -- 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]
