viirya commented on PR #51: URL: https://github.com/apache/spark-connect-rust/pull/51#issuecomment-5397718149
Re-reviewed after the updates. Two of the three things I raised here are fixed, and I verified them in the code rather than trusting the commit messages. One is fixed only partially, in a way that leaves the case I'd called the worst still broken. ## Confirmed fixed - **The stack is real now.** `git merge-base --is-ancestor` succeeds against #50's branch, so this genuinely builds on PART 1 and fixes pushed there will flow here. Thanks for rebasing. - **The GIL is released across the transport RPCs.** Every `block_on` in `transport.rs` is now wrapped — `connect`, `execute_plan`, `reattach_execute`, `release_execute`, `analyze_plan`, `config`, `interrupt`, `fetch_error_details` all go through `py.detach(...)`, matching what `__next__` already did. - **`PyDataFrame` too.** `collect`, `count`, `show`, `first`, `head`, `take`, `to_arrow`, `to_local_iterator`, and the iterator's `next` all detach. That's the hot path covered properly. - **`row.rs`** is down to a single `unwrap`, so the FFI-unwind concern is largely gone. ## Still open: `session.rs` holds the GIL, including the case I flagged as worst `crates/pyspark-rs/src/session.rs` contains **zero** `detach` calls, and several of its methods do network I/O: - `get_or_create` (line 50) — calls `builder.get_or_create()` while holding the GIL. This is the specific case I singled out last time: it blocks the entire interpreter through the connect handshake, so a server that isn't answering makes the process unresponsive — signal handling included, so Ctrl-C may not land until timeout. - `sql` (line 115) — `self.session.sql(query)` issues an AnalyzePlan round-trip. - Plus `stop`, `version`, `table`, `interrupt_all` / `interrupt_tag` / `interrupt_operation`, `add_artifact` / `add_artifacts`, `build_resource_profile`. `interrupt_all` and `add_artifacts` reach `block_on(self.client...)` in the core, so they're unambiguously blocking. `interrupt_*` is the sharpest of these in practice. Interrupt exists to cancel a query running on another thread, but the caller cannot acquire the GIL to *invoke* it while the blocked worker holds it — so on the current code the cancellation path is liable to be unavailable exactly when it's needed. (`transport.rs`'s `interrupt` does detach; it's the session-level wrapper that doesn't.) The fix is the same shape as the one already applied to `dataframe.rs` — these methods need a `py: Python<'_>` parameter so they can wrap the call in `py.detach(...)`. Given how consistently `dataframe.rs` was fixed, I suspect `session.rs` was simply missed rather than deliberately skipped. Worth noting neither the old nor the new behaviour here is observable in the official suite, which is effectively single-threaded — so this needs a deliberate test (two threads, one slow query, assert the other makes progress) rather than relying on the parity gate. ## On the new surface A lot arrived since the last pass — Structured Streaming exposure, `toLocalIterator`, `ResourceProfile`, `dataSource.register`, avro/protobuf/sha2/window dispatch, vendored `pyspark.resource` and `pyspark.sql.datasource`, datetime/Decimal literals, and a WASM UDF packer (`python/pyspark_wasm_udf/`). I haven't reviewed that in depth and I don't want to imply I have. Two structural observations rather than defects: The vendored trees (`pyspark.resource`, `pyspark.sql.datasource`, and the earlier `pyspark.pandas`) still raise the provenance question from my last comment: whether they're byte-identical to upstream (so license headers and a per-release sync story are what matter) or modified (in which case the diffs are the interesting part). A note in the PR description saying which, and how they get re-synced when Spark cuts a release, would save every future reviewer the same investigation. And the WASM UDF work spans both PRs — `wasm_udf.rs` and the macro/build crates on #50, the Python packer here. It's a genuinely separate capability from "drop-in pyspark wrapper," with its own runtime prerequisite (`wasmtime` on the executors). It would be easier to evaluate, and easier to revert if it doesn't pan out, as its own PR on top of this stack. I've filed the workspace-membership problem it causes (`cargo test --workspace` fails without the wasm32 target) on #50, since the crates live there. Nothing else from my earlier comment has changed status: the plan-builder bugs are fixed on #50, so the wheel no longer inherits them, and the test-harness boundary (the official suite builds plans with upstream pyspark and never executes `plan.rs`) still holds — I've left the follow-up on that with the golden-test discussion on #50. -- 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]
