namanjain24-sudo commented on PR #25318: URL: https://github.com/apache/datafusion/pull/25318#issuecomment-6710580151
Thanks for the detailed review. The comments were helpful. I've gone through each one and the PR is now 210 lines smaller. **The enum:** Removed. `ScalarSubqueryResults` now just holds an `Arc<dyn ScalarSubqueryResultsBackend>`, with the in-memory slots implementing the same trait as any other backend. The "remote" distinction is gone too. **The in-crate `forced_foreign` test:** Dropped. You're right that it was duplicating the cross-library test. That cross-library test runs in `cargo test (macos-aarch64)`, which is skipped for fork PRs but runs in the merge queue. **Fixtures in the FFI crate:** These need to stay in the crate because the test loads this crate's cdylib and asks it for the codec. The cdylib needs to be able to build the fixtures itself; types defined only in the test binary aren't available inside it. I've narrowed the gate from `any(test, feature = "integration-tests")` to just the feature. **The upgrade note:** Moved to a new `57.0.0.md`. It now simply says the context wasn't sufficient and points to #25089 for the details. The "who is affected" section is also down to the two lines you suggested. I removed the `ComposedPhysicalExtensionCodec` detail, the FFI rebuild paragraph (since that's generic to FFI layout changes), and the closing paragraph about the results container. **The remaining comments:** Trimmed them down to explain the purpose of each thing: the FFI field, the cross-library test, and `FFI_ScalarSubqueryResults`. For the last one, I also removed the "unlike most FFI wrappers" comparison rather than adding more explanation that doesn't really help the reader. There are two things I'd still like your input on: **`ScalarValueEnvelope`:** Your diagnosis is right. `ScalarValue::Float16` is currently written into `float32_value`, so a round trip widens the type. Expanding `protobuf::ScalarValue` seems like the right fix, but that would mean changing a shared wire format. Would you prefer that I make that change in this PR, or file it separately and remove the envelope once it lands? **The framing:** I agree that the PR's framing could be clearer. The trait change itself is 42 changed lines in `datafusion/proto/src/physical_plan/mod.rs`. The FFI transport needed to carry the context across is another 848 lines, plus 315 lines of tests. Would it make more sense to land the trait change separately and bring the FFI part in a follow-up? -- 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]
