HyukjinKwon commented on PR #50:
URL:
https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5401465199
Thanks for the third pass — all four actionable items are addressed. Commits
are on `HyukjinKwon:split-rust-core`.
## Golden tests for the five plan-builder bugs (the item you'd hold on)
Added `crates/spark-connect/tests/plan_arg_regression_golden.rs` — 12 tests
that assert the previously-dropped argument reaches the proto, so reintroducing
any of the five turns CI red:
- `dropna(how="all")` → asserts `DropNa.min_non_nulls == Some(1)` (plus
`how="any"` → `None` and explicit `thresh` → `Some(3)`).
- `hint` **with** parameters → asserts `Hint.parameters` contains the
serialized values (len 2, `Long(10)` + `String("name")`); the empty-param case
is kept separately.
- string `replace` → asserts `Replace.replacements[0].old_value/new_value`
are the `String` literals.
- `pivot` with explicit values → asserts `Aggregate.pivot.values` carries
them (len 2).
- `fillna` double → asserts `NaFill.values[0]` is `Double(1.5)` (plus
long/string cases).
These are inline proto-field assertions rather than `.jsonl` golden captures
precisely so they pin the argument→proto mapping without needing a live server,
and they generalize to the "mode/how/parameters argument dropped" class you
flagged.
## Retry budget now spans the whole operation + elapsed-time ceiling enforced
`RetryPolicyState` is hoisted to a `ReattachableResponseStream` field, built
once with the stream; the `message()` error arm reuses `&mut self.retry_state`
instead of constructing a fresh one per drop. So the attempt count and
escalating backoff persist across successive mid-stream reattaches — a flapping
stream now hits the global bound instead of resetting to
15/`initial_backoff_ms` each time.
I also wired `DEFAULT_MAX_RETRY_EXCEPTION_ELAPSED_TIME`: `RetryPolicy`
gained `max_retry_exception_elapsed_time_ms` (default = the 1-hour constant),
`RetryPolicyState` records a `started: Instant`, and `next_attempt()` returns
`None` once the elapsed ceiling is exceeded — with a unit test (`max_..._ms:
Some(0)` → immediate `None`). It's no longer an exported-but-unread guarantee.
## `cargo test --workspace` without wasm32
`wasm-udfs` and `wasm-udf-inline` are moved from `[workspace] members` to
`[workspace] exclude`, so `--workspace` no longer tries to build them without
the target; CI builds them explicitly via `cargo build --manifest-path
wasm-udfs/Cargo.toml` (the `rustup target add wasm32-unknown-unknown` step is
unchanged). `cargo build --workspace` / `cargo test --workspace` now succeed on
a clean checkout without wasm32. The build script also degrades from an opaque
`panic!` to a `cargo:warning` naming `rustup target add wasm32-unknown-unknown`.
On `lint.yml`'s `clippy … || true`: left as advisory for now — there are
~490 pre-existing clippy warnings, so flipping it to a gate would just fail CI
on unrelated lint. Flagging it as a separate cleanup rather than folding a
large lint pass into this stack.
## WASM UDF
Thanks for the careful read (and for re-checking the `SQL_ARROW_BATCHED_UDF`
choice). Two of your points are addressed:
- **Docs**: the README now states plainly that this one feature gives the
otherwise-pure-Rust client a **Python build-time dependency** (`python -m
pyspark_wasm_udf.pack`, needing `cloudpickle` + `pyspark`), and that the goal
is **capability, not speed** (Python worker → `wasmtime` → linear memory →
per-row entrypoint, not expected to beat a vectorized Arrow UDF).
- **Own PR / round-trip tests**: the project owner's call is to keep WASM in
this stack for now (happy to revisit). On the encode/decode round-trip test per
`AbiType` including the null/empty-array edges — you're right it deserves one;
a true round-trip needs `wasmtime` + a locally compiled module, which is
exactly what the `rust.yml` `wasm-udf` job already provisions, so that's where
it belongs. I'll add it there as a follow-up rather than a descriptor-only test
that wouldn't catch a pointer-arithmetic bug.
The `spark-connect-rs` deprecation path, `.asf.yaml` Issues-off, and the
4.2.0-vs-alpha framing are intentional decisions by the project owner (a
committer) and unchanged; noted.
--
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]
