HyukjinKwon commented on PR #64: URL: https://github.com/apache/spark-connect-rust/pull/64#issuecomment-5449537259
Thanks for the approval and for re-verifying from scratch. You're right that the body had drifted from `13ad5e1` — I've updated the description to match the code: - **What changes**: now states the 128 MiB cap is applied *unconditionally* to the stub + the three raw `Grpc` calls, matching the reference's `GRPC_DEFAULT_OPTIONS`; no `grpc_max_message_size` connection param, no `ChannelBuilder::max_message_size()`, `channel.rs` net-empty. Added a short note recording that the parameter approach was dropped after review and why. - **User-facing change**: corrected — it no longer claims the client "honors `grpc_max_message_size`"; that param isn't recognized. It now just raises the cap to 128 MiB to match the reference. - **How tested**: corrected to 93 `apache-spark-connect-core` tests (the param-parsing test was removed with the parameter), notes the cap rests on the tonic API contract, and records the >4 MiB parity check as follow-up. On the two non-blocking notes: agreed on keeping the constant inline at the four sites — a `fn raw_grpc(&self)` wrapper is the right move if a fifth `Grpc::new` site ever appears, but not worth it at four. And I'll confirm a parity test returns a >4 MiB `collect()` batch to close the end-to-end gap as 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]
