timsaucer commented on PR #19305: URL: https://github.com/apache/datafusion/pull/19305#issuecomment-3655559152
> **Re: Missing Tests** I double-checked and the tests are included in `datafusion/ffi/src/session/mod.rs`. They might be hidden in the diff because the file is large, but they extend `test_ffi_session` to verify the props are carried over. If you're referring to the two assert statements, I do not believe those are sufficient to demonstrate adequate coverage for this feature. They are also easy to overlook because in the diff you have implementation code after the test code. This is contrary to industry best practices. I am disinclined to spend more time reviewing this PR. It does not follow the established patterns in the FFI crate and requires significant rework to get it up to the standards of the rest of the crate. Thank you for taking the time to work on it, but I do not think this PR is anywhere close to being ready. -- 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]
