HyukjinKwon commented on PR #51:
URL:
https://github.com/apache/spark-connect-rust/pull/51#issuecomment-5401469609
Thanks — the GIL item and the provenance ask are both addressed. Commit on
`HyukjinKwon:split-python-wrapper`.
## `session.rs` GIL
You were right that it read as a missed file. Every blocking RPC in
`session.rs` now releases the GIL via `py.detach(...)`, matching
`dataframe.rs`/`transport.rs` (9 detach sites):
- `get_or_create` (the connect handshake — the one you singled out),
- `interrupt_all` / `interrupt_tag` / `interrupt_operation` — so the
cancellation path is available while a worker thread blocks, which was the
sharpest concern,
- `stop`, `version`, `add_artifact` / `add_artifacts`,
`build_resource_profile`.
Two you listed that I deliberately did **not** change, because in this
client they don't do I/O:
- `sql` — returns a lazy `DataFrame` wrapping `LogicalPlan::Sql`; it's
infallible and issues no AnalyzePlan. The round-trip happens on the action
(`collect`/`show`/…), which already detaches in `dataframe.rs`. (This differs
from reference pyspark, where `spark.sql()` can execute eagerly for commands —
here `sql` is a pure transformation.)
- `table` — likewise lazy (`read().table()`), no RPC.
If you'd rather `sql`/`table` detach defensively anyway I can add it, but as
written there's no blocking call to wrap.
On the two-threads regression test (one slow query, assert the other
progresses): agreed it's the right test and that the parity suite won't
exercise it since it's effectively single-threaded. It needs a live server, so
I can't run it in the offline/parity harness here; tracking it to add once the
fork CI has a server, and it would also guard the `dataframe.rs` detaches from
regressing.
## Provenance of the vendored trees
Added a "Provenance" section to the PR description. Short version:
`pyspark.pandas`, `pyspark.testing`, `cloudpickle`,
`pyspark.resource/{__init__,information,requests}.py`, and
`pyspark.sql.datasource.py` are **verbatim** from Apache Spark with headers
intact (re-synced from the matching release tag). The **adapted** files — where
the interesting diff lives — are `pyspark/resource/profile.py` (routes
`ResourceProfile.id` through the `_pyspark` extension instead of a JVM
builder), the `pyspark/sql/streaming/{query,readwriter}.py` re-exports +
client-side listener bus, and the minimal
`pyspark/sql/streaming/{listener,datasource}.py` subsets.
## WASM packer / own PR
Same as on #50: keeping WASM in the stack is the project owner's call for
now, and the encode/decode round-trip test per `AbiType` (incl.
null/empty-array) belongs in the `wasm-udf` CI job that has `wasmtime` + a
compiled module — adding it there as a follow-up rather than a test that can't
actually round-trip.
The newer surface you flagged as un-reviewed (streaming, `toLocalIterator`,
`ResourceProfile`, `dataSource.register`, avro/protobuf/sha2/window dispatch,
datetime/Decimal literals) — no expectation that you've signed off on it; happy
to walk through any of 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]
