andygrove commented on PR #5607: URL: https://github.com/apache/datafusion-comet/pull/5607#issuecomment-5719747771
Triage note: #5874 covers `length` on `BinaryType` too, and reaches the opposite conclusion. It registers `datafusion-spark`'s `SparkLengthFunc` under `length` / `char_length` / `character_length` / `len` and deletes the `BinaryType` gate from `CometLength` outright, so binary length runs natively rather than staying on Spark fallback because the dispatcher benchmarked slower. The two collide fairly directly. Both edit `CometLength` in `strings.scala`, both rewrite the notes in `bit_length.sql` and `octet_length.sql`, both add binary cases to `length.sql` — this PR asserting `expect_fallback(Length on BinaryType is not supported)` and #5874 asserting it runs natively — and both add a file called `CometBinaryLengthBenchmark.scala`. If the `datafusion-spark` kernel really is Spark-compatible for binary, native should beat both the dispatcher and the fallback, which would leave this PR as `bit_length` / `octet_length` plus the benchmark and the documentation of why the dispatcher is not worth it there. Could you and @dwsmith1983 settle the ordering before either gets more review? -- 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]
