namanjain24-sudo commented on PR #25318: URL: https://github.com/apache/datafusion/pull/25318#issuecomment-6761263873
Thanks - that was a fair call. I went back through the whole diff rather than just the three spots, and removed 55 lines of comments plus the code below. Net 175 lines out, 20 in. **The comments.** You were right that they were restating the code. The block you pointed at on `datafusion_ffi_test_create_scalar_subquery_expr_exec_codec` went from 31 lines to 7 - it had also gone stale, still referring to a unit test I had deleted. Three inline comments in the cross-library test were saying what the test's own doc comment already said, or narrating the code directly underneath them; those are gone. What is left is the reasoning that is not in the code: why `execute` evaluates the expression directly instead of pulling a batch from its child, and why `try_decode_fn_wrapper` builds a root context. I also stopped exposing the fixtures' internals - `ScalarSubqueryExprExecProto` and two struct fields were `pub` for no reason. **The two tests in `scalar_subquery_results.rs`.** Both gone. One covered `ScalarValueEnvelope`, which is also gone - I took your earlier point and dropped it, so a `Float16` now widens to `Float32` across the boundary like it does anywhere else `protobuf::ScalarValue` is used. That file is down from 294 lines to 177. One honest gap: the other test also exercised `set` and `clear` from the foreign side, and the integration test does neither. In the real flow the host populates the results and the far side only reads, so I do not think they are worth their own test, but say the word and I will put it back. **The fixtures' location.** They cannot live in `integration-tests/tests/cases/physical_extension_codec.rs` - that is the test binary, and the test loads a cdylib. But I was wrong in my last reply about *which* cdylib: `find_cdylib` looks for `libdatafusion_ffi_integration_tests`, not this crate's. So they could live in that crate's `src/lib.rs`, which is what gets built into the cdylib. The cost is that `datafusion-ffi-integration-tests` has exactly one entry in `[dependencies]` today; the fixtures would need seven of its dev-dependencies promoted, plus `prost` added. Keeping them here follows `datafusion_ffi_test_create_exec_with_byte_metrics`, which sits in `datafusion_ffi::tests` and reaches the cdylib the same way. Happy to move them if you would rather the test code not sit in the published crate - it is your call and I did not want to guess. **On the volume.** The fixtures module is 195 lines, and about 100 of those are the plan itself. Its `ExecutionPlan` impl is 58 of them, and six of the seven methods it implements are required by the trait. The part that could genuinely go is the plan altogether - the codec could assert inside the cdylib and return a trivial plan, which would cut those ~100 lines at the cost of a weaker assertion. And the structural version of the same question, which I asked last time and is still open: the trait change itself is 42 lines in `datafusion/proto/src/physical_plan/mod.rs`. I am happy to land that alone and bring the FFI transport as a separate PR, if that is an easier thing to review. -- 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]
