viirya commented on PR #50:
URL:
https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5392429230
Re-reviewed this as PART 1 of the split, carrying over the findings from #49
that live in the Rust core. I checked each against this branch rather than
assuming — all of them still apply here, at shifted line numbers.
Two notes on the split itself before the findings.
**This isn't a pure split of #49 — new work came with it.** Comparing the
stack's final state against #49 shows roughly +3,800/-900 across 27 files,
including a new `crates/spark-connect/src/merge.rs` (219 lines) and about +800
lines in `functions.rs`. That's fine, but it means the split isn't a no-op
re-shuffle and the added surface hasn't been reviewed before. Calling it out so
nobody assumes #49's review transfers automatically.
**#51 is not actually stacked on this branch.** It declares
`split-rust-core` as its base, but the two branches share no lineage — `git
merge-base --is-ancestor` fails, and both fork independently from `4f43dfa` on
master. #51 carries its own full 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, but as-is #51 can't merge as one —
landing this PR then #51 would conflict or silently revert core changes
depending on merge order. Worth rebasing #51 onto this branch so the stack is
real.
## The crate names are already taken on crates.io, so `release.yml` cannot
work
This 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.
That needs either a name-transfer conversation with both owners or a different
naming scheme (e.g. an `apache-*`-prefixed set) — better settled before this
lands, since the names are baked into every `Cargo.toml`, doc example, and
README snippet.
Separately, the outgoing **`spark-connect-rs` (0.0.2) gets no deprecation
path**. Existing 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 was restructured under different names. A final `0.0.3`
marked deprecated and pointing at the successor would close that out.
## Silent-wrong-result bugs in the plan builder
(`crates/spark-connect/src/plan.rs`)
All the same shape: an argument is accepted by the API, then dropped before
it reaches the proto. Nothing errors; the query just returns a wrong answer.
- **`dropna(how=...)` ignores `how` entirely.** `dataframe.rs` computes
`how_str` and stores it on `LogicalPlan::NADrop`, but `plan.rs:740`
destructures it as `how: _` and forwards only `thresh` → `min_non_nulls`.
PySpark derives `min_non_nulls` from `how` client-side; that translation is
absent, so `dropna(Some("all"), …)` and `dropna(Some("any"), …)` build
byte-identical plans and both behave as `"any"`.
- **`hint(name, parameters)` drops all parameters.** `plan.rs:692`
destructures `parameters: _` and never populates `hint.parameters`, though the
proto field exists (`relations.proto`, `repeated Expression parameters = 3`).
`df.hint("REPARTITION", [10])` sends a parameterless hint.
- **`replace()` silently no-ops on non-numeric values.** `plan.rs:764,772`
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:444` sets
`pivot.col` but never serializes `pivot_values` ("In the future, if
pivot_values are passed…"), and `group.rs` hardcodes `pivot_values: vec![]`
with no API to supply them.
- **`fillna` accepts only `i64`**, so a fractional fill into a double column
isn't expressible. Lower severity — an API gap rather than a wrong plan — but
it undercuts the parity claim.
Note these aren't confined to this PR's audience: PART 2 re-exports this
same plan builder to Python (`from pyspark._pyspark import DataFrame`), so the
published wheel inherits every one of them. Fixing them here fixes both.
## Nothing in the test setup can catch that class of bug
The golden tests (`tests/golden/*.jsonl`) are the only thing covering the
Rust plan builder, and none of the five above fall in their scope — which is
why they've survived this long. Given `plan.rs` is ~1,400 lines and
`functions.rs` is now over 4,000, the open question isn't these five; it's how
many more of the same shape exist with no test that would notice. A golden case
per DataFrame method that takes a mode/how/parameters argument would close the
category rather than the instances.
`scripts/audit_no_stubs.sh` compounds this by giving false confidence. It
runs in CI to enforce "no deferrals," but its regex misses the `plan.rs` pivot
deferral ("In the future…") and deliberately excludes bare
"placeholder"/"stub". A passing audit isn't evidence of completeness, and the
pivot gap is the proof.
## Retries and reattach are unwired
The description lists the core's responsibilities as "transport: channel,
retries, reattach," and `reattach.rs` opens with "Implements the full reattach
protocol." Neither is actually connected:
- `RetryPolicy` / `RetryPolicyState` (`retries.rs`) are only `pub use`d —
`client.rs` has zero references to them. Every RPC 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.
This is a regression against the crate being replaced. The outgoing
`spark-connect-rs` did drive reattach for Rust callers — its `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. Here, `DataFrame::collect()` (`dataframe.rs:441`) iterates the
raw stream with no retry and no reattach, so a transient `UNAVAILABLE` or
`INVALID_CURSOR.DISCONNECTED` aborts a query the old crate would have resumed.
Either wire these modules in, or drop the "retries/reattach" framing and delete
the dead iterator so it stops implying coverage that isn't there.
## Scope and governance
This deletes the entire existing `spark-connect-rs` crate (~19k lines) and
replaces the project's architecture and identity. A couple of non-code
decisions ride along that I think belong in a dev@ thread rather than a
restructure PR:
- **`.asf.yaml` turns GitHub Issues off** (`issues: true` → `false`),
closing an existing intake channel and redirecting reporters to JIRA.
- The crates 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 signals point in opposite directions for anyone deciding whether to
depend on this.
To be clear on the architecture itself: I think the layering is a genuine
improvement — splitting proto / transport / DataFrame across crate boundaries
is a real upgrade over the old single crate's module split. My blocking
concerns are the crates.io naming (nothing can be published as-is) and the
plan-builder bugs, since those return wrong answers with no error surfaced.
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]
