viirya commented on PR #51: URL: https://github.com/apache/spark-connect-rust/pull/51#issuecomment-5398144026
Third pass. One item from my last comment is still open, and it's the case I'd previously singled out as the worst of the set. ## Still open: `session.rs` holds the GIL `crates/pyspark-rs/src/session.rs` contains **zero** `detach` calls, while `dataframe.rs` and `transport.rs` were both fixed thoroughly. Given how consistent those two are, this reads as a missed file rather than a decision. The methods that block while holding the GIL: - **`get_or_create` (line 50)** — the one I flagged last time. It runs the connect handshake with the GIL held, so a server that isn't answering freezes the whole interpreter, signal handling included; Ctrl-C may not land until the timeout expires. - **`sql` (line 115)** — issues an AnalyzePlan round-trip. - `stop`, `version`, `table`, `interrupt_all` / `interrupt_tag` / `interrupt_operation`, `add_artifact` / `add_artifacts`, `build_resource_profile`. I confirmed `interrupt_all` and `add_artifacts` reach `block_on(self.client...)` in the core, so those are unambiguous. `interrupt_*` is the sharpest in practice, and worth calling out as more than a performance nit. Interrupt exists to cancel a query running on another thread — but the caller can't acquire the GIL to *invoke* it while the blocked worker holds it. So the cancellation path is liable to be unavailable exactly when a user needs it. (`transport.rs`'s `interrupt` does detach; it's the session-level wrapper that doesn't.) Same fix as `dataframe.rs`: thread a `py: Python<'_>` parameter through and wrap the call in `py.detach(...)`. This won't surface in the official suite, which is effectively single-threaded, so it needs a deliberate test — two threads, one slow query, assert the other makes progress — rather than relying on the parity gate. That test would also protect the `dataframe.rs` fixes from regressing. ## Confirmed fixed since last pass - **The stack is real.** `git merge-base --is-ancestor` now succeeds against #50's branch, so fixes there flow here. - **GIL released across the transport RPCs** — all of `connect`, `execute_plan`, `reattach_execute`, `release_execute`, `analyze_plan`, `config`, `interrupt`, `fetch_error_details` go through `py.detach(...)`. - **`PyDataFrame`** — `collect`, `count`, `show`, `first`, `head`, `take`, `to_arrow`, `to_local_iterator`, and the iterator's `next` all detach. - **`row.rs`** is down to one `unwrap`, so the FFI-unwind concern is essentially closed. - The plan-builder bugs are fixed on #50, so the published wheel no longer inherits them. ## On the WASM UDF packer The Python side of the WASM UDF feature lives here (`python/pyspark_wasm_udf/`), with the Rust half on #50. I've put my substantive feedback on that PR since the crates are there, but two points touch this one: The runner in `__init__.py` does the actual work — encoding each row's arguments into wasm linear memory, calling the entrypoint, decoding the result — and it's hand-rolled pointer arithmetic (`packed >> 32`, `& 0xFFFFFFFF`, explicit `alloc`/`dealloc`). It has no tests. A round-trip test per `AbiType`, including the null and empty-array cases, would be cheap relative to the risk; a decode bug here produces silently wrong column values rather than an error. I'd also suggest the whole feature move to its own PR stacked on this one. It's a distinct capability from "drop-in pyspark wrapper," it spans both PRs, and it carries a deployment prerequisite (`wasmtime` on every executor) that a wrapper PR shouldn't be quietly introducing. Reviewing the ABI and the by-value cloudpickling on their own merits would be easier there, and it'd be revertible without touching the wrapper. ## On the newer surface Structured Streaming exposure, `toLocalIterator`, `ResourceProfile`, `dataSource.register`, avro/protobuf/sha2/window dispatch, datetime/Decimal literals, and the vendored `pyspark.resource` / `pyspark.sql.datasource` / `pyspark.pandas` trees all arrived across the last two passes. I haven't reviewed that in depth and don't want to imply otherwise. The vendored trees still raise the provenance question from my first comment: whether they're byte-identical to upstream (so license headers and a per-release re-sync story are the concern) or modified (in which case the diffs are the interesting part). A line in the PR description stating which, and how they get re-synced when Spark cuts a release, would save every future reviewer from re-deriving it — and it's the kind of thing an ASF release audit will ask about anyway. Finally, on the test-harness boundary from my first comment: now that the plan-builder bugs are fixed on #50, this is no longer urgent, but the structural point stands — the official suite builds plans with upstream pyspark and never executes `plan.rs`, so it can't defend the layer that ships. I've left that discussion with the golden-test item on #50 rather than duplicating it. -- 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]
