dwsmith1983 commented on PR #5874: URL: https://github.com/apache/datafusion-comet/pull/5874#issuecomment-5666324196
> Could you add a short comment at the registration recording what it depends on? ... Would it be better to assert through `create_scalar_function_expr` so the test follows the path the planner actually takes? Both done. The registration comment records that `SparkLengthFunc` unwraps no dictionaries and gets no planner coercion, so it relies on `ScanExec`, the shuffle scan and the Parquet adapter delivering plain `Utf8` or `Binary`. The test now builds the `ScalarFunc` proto over a bound column and plans it through `PhysicalPlanner::create_expr`, asserting an `Int32` scalar function over the column with no cast for both string and binary; the wider and view encodings keep a return-type check only, since no Comet input path produces them. The branch is merged with `main` and the `jni_api.rs` conflict resolved. > Could you and @adibmbrk agree on which PR owns `length` and have the other drop that part? I agree this PR should own `length`, for the reason you give: a native kernel over routing through the dispatcher. @adibmbrk, would you drop the `length` part and the `length.sql` edits from #5607 and keep `bit_length` and `octet_length` there? I am happy to borrow `CometBinaryLengthBenchmark` for the numbers @sunchao asked for rather than add a third harness. -- 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]
