andygrove commented on PR #3591: URL: https://github.com/apache/datafusion-comet/pull/3591#issuecomment-3959543708
Thanks for wiring up these two expressions. The serde and registration changes look correct to me. I have a couple of suggestions on the test side. The project has been moving expression tests to the SQL file-based framework (`CometSqlFileTestSuite`) rather than adding to Scala test suites. You can see examples in `spark/src/test/resources/sql-tests/expressions/datetime/` (like `make_date.sql`). Would you be open to moving these tests to SQL files instead? The framework automatically runs each query through both Spark and Comet and compares results, so you don't need the `checkSparkAnswerAndOperator` calls. There's a guide at https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html if that helps. It would also be good to add some null handling coverage. Both expressions are null-intolerant per the Spark spec, so testing with NULL column values (not just non-null data) would help catch any issues. Something like inserting a row with NULLs in the test table would do it. -- 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]
