viirya commented on PR #50: URL: https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5397717916
Re-reviewed after the updates. Every item I raised on the Rust core has been addressed, and I checked the fixes against the code rather than taking the commit messages at face value — they're real fixes, not cosmetic ones. Details below, then three new findings from the material that arrived with them. ## Confirmed fixed - **`dropna(how=...)`** — now derives `min_non_nulls` properly (`plan.rs`): explicit `thresh` wins, else `how="all"` → `Some(1)`, `how="any"` → `None`. Matches the pyspark semantics I described. - **`hint(name, parameters)`** — parameters are serialized, with an integer-looking value becoming a `Long` literal and everything else a `String`. - **`replace()`** — `str_to_proto_literal` now always sets both `old_value` and `new_value`, so string replacements are no longer a silent no-op. - **Explicit-value `pivot`** — `pivot_values` are serialized as literals, with the empty case correctly documented as "server computes distinct values." - **`fillna`** — `fillna_double` / `fillna_string` / `fillna_bool` cover the typed variants over a shared `fillna_value`. - **Retries** — genuinely wired: `client.rs` holds a `RetryPolicy`, and `with_retry` wraps the unary RPCs with backoff via `RetryPolicyState`. - **Reattach** — the dead iterator is now driven. `ReattachableResponseStream::message()` resumes via `ReattachExecute` from the last `response_id` on a retriable mid-stream drop, and `DataFrame::collect()` goes through `execute_plan_reattachable`. This closes the regression against the outgoing crate. - **`audit_no_stubs.sh`** — pattern extended with `[Ii]n the future` and switched to `-i`, so the deferral class that slipped through is now caught. That's a thorough response; the reattach driver in particular is more than a spot-fix. ## New: the retry budget resets on every mid-stream drop `ReattachableResponseStream::message()` constructs `RetryPolicyState::new(...)` *inside* the error arm, so each disconnect starts a fresh budget of `max_retries` (default 15) rather than drawing from one budget for the operation. A stream that drops repeatedly — a flapping proxy, a server recycling connections under load — can therefore reattach indefinitely: 15 attempts, one success, drop, 15 more, and so on, with the backoff also restarting at `initial_backoff_ms` each time so it never escalates. The intended ceiling is never reached and a hung query has no natural end. PySpark scopes one `Retrying` instance to the whole operation for exactly this reason. Hoisting the `RetryPolicyState` to a struct field (constructed alongside the stream, reset only on a successful `ResultComplete`) would restore the bound. Worth also considering `DEFAULT_MAX_RETRY_EXCEPTION_ELAPSED_TIME` — it's defined and exported but nothing reads it, so the one-hour elapsed-time ceiling isn't enforced anywhere. ## New: `cargo test --workspace` fails without the wasm32 target The WASM UDF crates are full workspace members: ``` members = [ ..., "wasm-udfs", "wasm-udf-inline" ] ``` and their build scripts hard-`panic!` when the target is absent (`crates/spark-connect-build/src/lib.rs:101`, "wasm32 build of … failed"). On a clean checkout without `rustup target add wasm32-unknown-unknown`: - `cargo test` (default-members) — passes, 100+ tests green. - `cargo test --workspace` — **fails to build**, `error[E0463]: can't find crate for 'std'`. CI happens to hide this: `rust.yml` installs the wasm32 target, so the break only bites contributors locally, which is the worst place for it. Given the feature is explicitly optional (`wasm-udf = ["dep:base64"]`, off by default, and it needs `wasmtime` on the executors), the WASM crates probably shouldn't be unconditional workspace members — or the build script should degrade to a clear skip instead of a panic when the target is missing. Two smaller things in the same area: `lint.yml:49` runs `cargo clippy --workspace --all-targets || true`, so clippy findings can never fail the build — that makes the lint job advisory only, which seems at odds with having it gate. And the WASM UDF subsystem is a substantial new capability (a proc-macro crate, a build crate, two example crates, a Python packer) that arrived during review of a restructure; it's independently reviewable and would be easier to evaluate on its own PR. ## Still open: the fixes have no tests covering the fixed behaviour This is the finding I'd most like to see closed, because it's the reason these bugs shipped in the first place. The fixes landed without golden cases that exercise the arguments that were being dropped: - `test_dropna_golden` (`dataframe_extra_golden.rs:28`) covers `df.range(5).dropna()` — no `how`, so it passes identically before and after the fix. - The golden `hint` case (`plans_golden.rs:592`) is `plan::hint(range, "broadcast", vec![])` — an empty parameter list, so it too passes either way. - I found no case asserting a string `replace`, an explicit-value `pivot`, or a `fillna_double`. So all five bugs would still pass CI today if reintroduced. The category matters more than the instances: `plan.rs` is ~1,500 lines and `functions.rs` is now past 4,000, and the shape of the bug — an argument accepted then dropped before the proto — recurs anywhere a method takes a mode/how/parameters argument. A golden case per such method, asserting the argument reaches the proto, would convert this from "five bugs found by reading" into a class the suite defends. Adding one for each of the five fixes here would be a good start. Everything else from my earlier comment (crates.io naming, `spark-connect-rs` deprecation, Issues-off, 4.2.0-vs-alpha) is unchanged as far as I can tell, so I won't repeat 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]
