viirya commented on PR #51: URL: https://github.com/apache/spark-connect-rust/pull/51#issuecomment-5392429440
Re-reviewed this as PART 2 of the split. The Rust-core findings from #49 are on #50 where they belong; this comment covers what's specific to the Python wrapper and the test harness. I verified each against this branch — all still apply. ## This branch isn't actually stacked on #50 It declares `split-rust-core` as its base, but the two branches share no lineage: `git merge-base --is-ancestor pr-50 pr-51` fails, and both fork independently from `4f43dfa` on master. This branch carries its own complete copy of the Rust core — all 21 files under `crates/spark-connect/src`, plus `release.yml` and `.asf.yaml`. GitHub's diff view makes it look like a clean follow-up, which hides two problems: merging #50 and then this would conflict or silently revert core changes depending on order, and any fix pushed to #50 won't appear here. A rebase onto #50's branch would make the stack real and shrink this diff to just the Python surface. Also worth flagging that the split wasn't purely mechanical: the final state differs from #49 by roughly +3,800/-900 across 27 files, including a new `crates/spark-connect/src/merge.rs` and ~800 new lines in `functions.rs`. That new surface hasn't been reviewed before, so #49's review doesn't transfer wholesale. ## The plan-builder bugs reach the published wheel `python/pyspark/sql/dataframe.py` is 28 lines whose substance is `from pyspark._pyspark import DataFrame` — it re-exports the Rust `PyDataFrame` directly. So a user who `pip install`s `pyspark-client-rust` and calls `df.dropna(how="all")` goes through `pyspark-rs` → `spark-connect` → the plan builder, and inherits every silent-argument-drop bug I filed on #50 (`dropna`'s `how`, `hint`'s `parameters`, `replace` on non-numerics, explicit-value `pivot`). For something positioned as a drop-in `pyspark` replacement, "same API call, quietly different answer" is the worst available failure mode — worse than an error, because nothing tells the user to look. The fixes belong on #50; I'm noting it here so the wheel's exposure is visible on the PR that ships it. ## The official test harness structurally cannot catch those bugs The parity harness is the main quality argument for this PR, but it exercises a different plan builder than the one that ships. `scripts/rust_transport_plugin.py` monkeypatches the upstream `SparkConnectClient` so its gRPC stub becomes `RustConnectStub`. In that path **upstream pyspark builds the plan** — `_RustStub.ExecutePlan(request)` receives an upstream protobuf object and calls `request.SerializeToString()`, and the bytes go out through `execute_plan_raw` + `BytesCodec` untouched. Rust's job there is transport and Arrow decoding only; `crates/spark-connect/src/plan.rs` never executes. So the paths diverge exactly where it matters: | Path | Who builds the plan | Exercises `plan.rs`? | |---|---|---| | Rust-native (`examples`) | Rust `spark-connect` | yes | | Published wheel (`python/pyspark/`) | Rust, via `_pyspark` | yes | | Official suite (`rust_transport_plugin.py`) | upstream pyspark | **no** | The code paths users run and the ones the official suite covers don't intersect on plan building. A green parity gate therefore says nothing about the `dropna`/`hint`/`replace`/`pivot` bugs — which is precisely why they reached review unnoticed. I want to be clear this isn't an argument against the harness: routing the official suite through the Rust transport is a genuinely good idea and it does validate the transport and Arrow paths well. The problem is only that it's being presented as parity evidence for a layer it bypasses. Making that boundary explicit in the docs — and covering plan building with golden tests instead — would keep the harness's real value without overclaiming. ## The GIL is held across blocking RPCs The streaming path gets this right: `ResponseStream::__next__` wraps the blocking read in `py.detach()` and races it against cancellation (`transport.rs:114`), and the comment there explains exactly why. The other call sites don't follow it — `transport.rs` lines 167, 193, 207, 220, 227, 235, 242, 251 (`connect`, `execute_plan`, `reattach_execute`, `release_execute`, `analyze_plan`, `config`, `interrupt`, `fetch_error_details`) all call `block_on(...)` while holding the GIL, as do the `spark-connect` crate's `collect`/`count`/`first`/`show` reached through `pyspark-rs`. Each of those is a synchronous network round-trip, so while one is in flight every other Python thread is blocked. Concretely: - Four threads each running `spark.sql(q).collect()` serialize completely instead of overlapping — threading buys nothing. - A background thread running a slow query stalls the main thread, so even a `print` progress loop freezes and the program looks hung. - `SessionBuilder.get_or_create` blocks the whole interpreter through the connect handshake, including signal handling, so a server that isn't answering can make Ctrl-C unresponsive until timeout. This matters more than usual for a drop-in replacement: real pyspark uses `grpcio`, which releases the GIL properly, so the same user code that scales across threads on upstream pyspark will serialize here. It also won't show up in the official suite, which is largely single-threaded. The fix is mechanical — wrap each `block_on` the way `__next__` already does; the `spark-connect` methods need a `py` handle threaded through first. Smaller item nearby: `value_to_py` (`crates/pyspark-rs/src/row.rs`) `.unwrap()`s every `into_pyobject`. These are near-infallible, but a panic there unwinds across the FFI boundary instead of raising a Python exception — `?` would be safer. ## On the `pandas` surface This branch vendors a large `python/pyspark/pandas/**` tree (~80 files). I didn't audit it in depth, but it's worth stating what the intent is: whether it's vendored verbatim from upstream (in which case its provenance and license headers need to be exact, and it will need a sync story per Spark release) or modified. If any of it is stubbed against the Rust core, the same "golden tests don't cover it" gap applies there too. Overall: the transport seam here is the strongest part of the change, and the strangler-fig approach is a smart way to get the official suite involved. My concerns are the unreal stack (this should be rebased onto #50), the wheel inheriting the plan-builder bugs, and the GIL behaviour — the last one being the item most likely to be reported as "the Rust client is slower than pyspark" once people run it under threads. -- 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]
