comphead commented on code in PR #24140: URL: https://github.com/apache/datafusion/pull/24140#discussion_r3736679166
########## datafusion/spark/README.md: ########## @@ -27,23 +27,39 @@ This crate is a submodule of DataFusion that provides [Apache Spark] compatible [apache datafusion]: https://datafusion.apache.org/ [apache spark]: https://spark.apache.org/ -## Testing Guide +## Implementation Guidelines -When testing functions by directly invoking them (e.g., `test_scalar_function!()`), input coercion (from the `signature` -or `coerce_types`) is not applied. +When implementing these functions, you can check if there are existing implementations +in the [Sail] or [Comet] projects first. If you do port functionality from these +sources, make sure to port over the corresponding tests too, to ensure correctness +and compatibility. -Therefore, direct invocation tests should only be used to verify that the function is correctly implemented. +### `simplify()` Review Comment: Ideally we need to guarantee parity between `simplify` and `invoke_with_args` however there is no flag the spark function is simplified to make a decision. We prob need to come up with solution to disable optimizer rules in slt. Spark already has this through `SQLConf.OPTIMIZER_EXCLUDED_RULES` I suppose we need similar for DataFusion, not only for Spark but in general. Having the mechanism it would be easier to solve questions like @andygrove mentioned. If you okay, I can come up with draft PR -- 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]
