viirya commented on PR #49:
URL:
https://github.com/apache/spark-connect-rust/pull/49#issuecomment-5392367636
I reviewed this end-to-end (the transport core, the PyO3 bridge, the
pure-Rust `spark-connect` DataFrame API, and the test harness). This is a
large, carefully-organized change and the transport seam in particular is
nicely done — the byte-passthrough stub is a clean way to let the official
suite exercise the Rust core. My concerns fall into four buckets: a publishing
blocker, some silent-wrong-result bugs in the plan builder, a gap between what
the test harness covers and what ships, and repo-governance scope.
## The crate names are already taken on crates.io, so `release.yml` cannot
work
This one blocks the release story regardless of code quality:
- **`spark-connect`** — the name this PR gives its flagship crate — is
already published on crates.io by an unrelated third party (currently 0.2.2,
`github.com/franciscoabsampaio/spark-connect`).
- **`spark-connect-core`** is also taken, by the previous architecture's
author (0.0.1-beta.5, `github.com/sjrusso8/spark-connect-rs`).
- Only `spark-connect-proto` is free.
`release.yml` claims to publish the Rust crates to crates.io, but `cargo
publish` will fail outright until the ASF obtains ownership of those two names.
This needs either a name-transfer conversation with both owners or a different
naming scheme (e.g. an `apache-*`-prefixed set) — worth settling before the
restructure lands, since the crate names are baked into every `Cargo.toml`, doc
example, and README snippet here.
Separately, the existing **`spark-connect-rs` (0.0.2) gets no deprecation
path**. Current users aren't broken — no new crate reuses that name, and
`0.0.2` → `4.2.0` won't be picked up by `cargo update` — but they'll also never
learn the project has been restructured under different names. A final `0.0.3`
marked deprecated and pointing at the successor crate would close that out.
Minor, but related: the project now answers to five different names
(`spark-connect-rust` the repo, `spark-connect` the crate, `spark-connect-rs`
the old crate, `pyspark-client-rust` on PyPI, `pyspark` as the import). The
three that differ by a few characters are a lasting source of confusion.
## Silent-wrong-result bugs in the plan builder
(`crates/spark-connect/src/plan.rs`)
These all follow the same shape — an argument is accepted by the API, then
dropped before it reaches the proto. No error surfaces; the query just returns
the wrong answer:
- **`dropna(how=...)` ignores `how` entirely.** `dataframe.rs:742` computes
`how_str` and stores it on `LogicalPlan::NADrop`, but `plan.rs:667`
destructures it as `how: _` and forwards only `thresh` → `min_non_nulls`.
PySpark translates `how` into `min_non_nulls` client-side (`"all"` → 1, `"any"`
→ column count); that translation is absent, so `dropna(Some("all"), None,
None)` and `dropna(Some("any"), None, None)` build byte-identical plans and
both behave as `"any"`.
- **`hint(name, parameters)` drops all parameters.** `plan.rs:619`
destructures `parameters: _` and never populates `hint.parameters`, though the
field exists (`relations.proto:969`). `df.hint("REPARTITION", [10])` sends a
parameterless hint.
- **`replace()` silently no-ops on non-numeric values.** `plan.rs:691,699`
only set `old_value`/`new_value` when the string parses as `f64`; otherwise the
`Replacement` is pushed with both fields `None`. `df.replace([("foo","bar")])`
on a string column does nothing at all, with no error.
- **Explicit-value `pivot` is unimplemented.** `plan.rs:394` sets
`pivot.col` but never serializes `pivot_values` (comment: "In the future, if
pivot_values are passed…"), and `group.rs:48` hardcodes `pivot_values: vec![]`
with no API to supply them.
- **`fillna` accepts only `i64`** (`dataframe.rs:723`), so a fractional fill
into a double column is not expressible. Lower severity — an API gap rather
than a wrong plan — but it undercuts the parity claim.
**These affect every real consumer, not just Rust users.**
`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 the published `pyspark-client-rust` and
calls `df.dropna(how="all")` goes through `pyspark-rs` → `spark-connect` → the
same `plan.rs` code above. Both the Rust-native path and the shipped Python
wrapper hit these bugs. For something positioned as a drop-in `pyspark`
replacement, "same API call, quietly different answer" is the worst available
failure mode.
## The official test harness structurally cannot catch the above
The parity harness is presented as the main quality argument, 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` is never executed.
So the three 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 (`scripts/rust_transport_plugin.py`) | upstream pyspark |
**no** |
The set of code paths users run and the set the official suite covers don't
intersect on plan building. A green parity gate therefore says nothing about
`dropna`/`hint`/`replace`/`pivot` — which is precisely why those four reached
this PR unnoticed. The golden tests (`tests/golden/*.jsonl`) are the only thing
covering the Rust plan builder, and none of the four are in their scope. Given
`plan.rs` is 1366 lines and `functions.rs` is 3351, the open question isn't
these four bugs — it's how many more of the same shape exist with no test that
could detect them.
`scripts/audit_no_stubs.sh` has the same false-confidence problem: it's run
in CI to enforce "no deferrals," but its regex misses the `plan.rs:394` pivot
deferral ("In the future…") and deliberately excludes bare
"placeholder"/"stub". A passing audit isn't evidence of completeness; the pivot
gap is the proof.
## Retries and reattach are unwired in the Rust core
The description lists the core's responsibilities as "transport: channel,
retries, reattach," and `reattach.rs` opens with "Implements the full reattach
protocol." In practice neither is connected on the Rust side:
- `RetryPolicy` / `RetryPolicyState` (`retries.rs`) are only `pub use`d. No
client method or DataFrame path calls `can_retry`/`next_attempt`; every RPC in
`client.rs` hits the stub directly with no retry.
- `ExecutePlanResponseReattachableIterator` (`reattach.rs`) is a data holder
with getters/setters and no stream-consumption loop. Nothing drives it. Real
reattach happens only because the *Python* client drives the raw passthrough.
Worth noting this is a regression against what's being replaced: the
outgoing `spark-connect-rs` did drive reattach for Rust callers —
`client/mod.rs` has the `while let Some(_) = stream.message().await` loop that
tracks `response_id`, watches for `ResultComplete`, and calls
`reattach_execute()` on interruption. Under this PR, `DataFrame::collect()`
(`dataframe.rs:347`) iterates the raw stream with no retry and no reattach, so
a transient `UNAVAILABLE` or `INVALID_CURSOR.DISCONNECTED` aborts a Rust-native
query that both the old crate and the Python path would have resumed. Either
wire these modules into the Rust client path, or drop the "retries/reattach"
framing for the core and delete the dead iterator so it stops implying coverage
that isn't there.
## PyO3: 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`). The unary and stream-opening calls don't: `execute_plan`,
`reattach_execute`, `analyze_plan`, `config`, `interrupt`, `release_execute`,
and `fetch_error_details` all call `block_on(...)` while holding the GIL, as do
the `spark-connect` crate's `collect`/`count`/`first`/`show`. Each is a
synchronous network round-trip, so in a multi-threaded Python program every
other thread stalls for its duration; `SessionBuilder.get_or_create` blocks the
whole interpreter through the connect handshake. Wrapping these the way
`__next__` already does would fix it.
Smaller item in the same area: `value_to_py` (`pyspark-rs/src/row.rs:24-37`)
`.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.
## Scope and governance
This deletes the entire existing `spark-connect-rs` crate (~19k lines) and
replaces the project's architecture and identity wholesale, so a couple of
non-code decisions ride along that I think deserve their own discussion on dev@
rather than being carried by a restructure PR:
- **`.asf.yaml` turns GitHub Issues off** (`issues: true` → `false`),
closing an existing intake channel and redirecting reporters to JIRA.
- The crates and wheel are versioned **4.2.0** to track the Spark release,
while the README says **"Status: alpha, work in progress"** and treats API
parity as a goal. Those two signals point in opposite directions for anyone
deciding whether to depend on this; the outgoing crate's honest `0.0.2` had the
opposite problem but at least matched its maturity.
On the architecture itself I want to be clear that I think the layering is a
genuine improvement — splitting proto / transport / DataFrame / bindings across
crate boundaries is a real upgrade over the old single crate's module split,
and the strangler-fig transport seam is a smart way to get the official suite
involved. My blocking concerns are the crates.io naming (nothing can be
published as-is) and the four plan-builder bugs, since those ship wrong answers
to every consumer while the headline test suite stays green.
Happy to file JIRAs for the individual bugs if that's the preferred tracking
path.
--
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]
