HyukjinKwon commented on PR #51: URL: https://github.com/apache/spark-connect-rust/pull/51#issuecomment-5393949844
Thanks — all the wrapper-specific findings are addressed (pushed here); the core findings were fixed on #50 and this branch is now genuinely stacked on it. **Stack is real now.** Re-synced this branch's base to #50's tip, so the diff is just the Python wrapper surface (no duplicated Rust core). **Plan-builder bugs reaching the wheel.** Fixed at the source on #50 (`dropna`/`hint`/`replace`/`pivot`/`fillna`, plus `createTempView`, the caching family, `LocalRelation` schema, etc.), so the wheel inherits the fixes. **Harness overclaiming parity.** You're exactly right that `rust_transport_plugin.py` routes plan-building through upstream pyspark, so the official suite exercises **transport + Arrow**, not our `plan.rs`. Updated the README CI section and added a "coverage boundary" note in `docs/design/OFFICIAL_TESTS_HARNESS.md`: parity is golden-proto tests (plan building) **plus** the official suite (transport/Arrow); neither alone is sufficient. No longer presented as end-to-end parity for the plan layer. **GIL across blocking RPCs — fixed.** Wrapped the `block_on` sites in `transport.rs` (connect/execute_plan/reattach/release/analyze/config/interrupt/fetch_error_details) and the blocking `PyDataFrame` actions (`collect`/`count`/`show`/`first`/`head`/`take`/`toArrow`) in `py.detach`, matching what `ResponseStream::__next__` already did — so Python threads no longer serialize on network round-trips. Also `value_to_py` now returns `PyResult` and propagates with `?` instead of `.unwrap()` panicking across FFI. **pandas surface.** `python/pyspark/pandas/**` is vendored verbatim from Apache Spark (license headers intact); what this PR adds is only the entry-point bridges (`DataFrame.pandas_api()` / `plot`) into it, not a reimplementation — so it carries the usual per-release sync obligation, and the "golden tests don't cover it" caveat is noted. Appreciate the review — the transport-seam feedback in particular made the harness's real (and its non-)coverage explicit in the docs. -- 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]
