HyukjinKwon commented on PR #54: URL: https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5420104798
<!-- ai-code-review --> ## Re-review at `814cdbd` — all findings resolved ✅ This commit addresses the remaining open items from the earlier passes. Verified each against the code: - **A (V2 writer option coercion) — RESOLVED.** `PyDataFrameWriterV2::option` now takes `&Bound<PyAny>` and both `option`/`options` route through `crate::coerce_option_value`, matching the V1 writer (None → option unset, bool → lowercase `"true"`/`"false"`). So `writeTo(...).options(mergeSchema=None)` no longer sends `"None"`. - **B1 (e2e suite didn't run in CI) — RESOLVED.** A new step in the `connect-parity` job (which already stands up the 4.2.0 server) runs `e2e_functions/columns/dataframe/coverage/integration` with `SPARK_REMOTE=sc://localhost:15002`, serially (`--test-threads=1`). No `|| true`, so a failure gates the job — the drop-in behavior is now genuinely verified in CI rather than silently skipped. (Author validated all 38 tests pass together against 4.2.0.) - **B2 (dropped server-free plan guard) — RESOLVED.** New `plan_serialization.rs` asserts the real proto mapping (not a snapshot) for every `JoinType` — including the `RightOuter`/`FullOuter`/`LeftSemi`/`LeftAnti` variants I flagged — plus the `on`-condition/`using_columns` forms, and `local_relation` (incl. inline-data bytes) / `cached_remote_relation` (relation id). It's server-free and runs unconditionally in CI. Signatures and proto paths check out. (Couldn't run `cargo test` in my sandbox — no crates.io access — but the author validated locally.) - **B3 (the `udf_builders` "free win"):** correctly reassessed — `get_or_create` connects eagerly, so that test isn't truly server-free; it now runs inside the new parity-job e2e step (`e2e_coverage`) instead. Reasonable. That closes every finding from all four passes (the #1 timestamp regression, the #2/#3 parity-gate holes, and #4–#10 / A / B). **Remaining are two pre-existing, out-of-scope follow-ups** (not introduced by and not required for this PR): `functions::cast(_col, to_col) -> to_col` is a no-op that `audit_no_stubs.sh` doesn't catch; and the `uuid`/`tuple_*`/datasketches builders emit a proto missing a trailing arg reference includes (which is why they're excluded from the golden). Worth separate JIRAs. **Verdict: no blockers; all review feedback addressed. LGTM.** This review was written by Isaac. -- 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]
