Jefffrey commented on code in PR #24140: URL: https://github.com/apache/datafusion/pull/24140#discussion_r3735036871
########## 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: This is one of the cases where Rust unit tests would be needed; though perhaps it also suggests that maybe we should stick to `invoke_with_args` only, and avoid `simplify` altogether especially in cases where it is an unconditional rewrite 🤔 -- 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]
