HyukjinKwon commented on PR #54: URL: https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5420072581
<!-- ai-code-review --> ## Re-review at `f6aacd2` Reviewed the new head. The change since `462912c` is one test-only commit (`f6aacd2`) extending the functions golden suite **440 → 508**. No regressions. **The golden extension is correct and genuinely strengthens CI coverage.** The 68 new builders are captured from a fresh reference-pyspark run and compared byte-for-byte (after normalization) against `tests/golden/functions.jsonl`. I checked the assertion loop (`functions_golden.rs`): a missing golden and a proto mismatch both record a failure, `assert!(failures.is_empty())` fails the test on any of them, and `assert_eq!(total, 508)` pins the count so a silently-dropped case also fails. It runs unconditionally (no `SPARK_REMOTE` gate), so unlike the `e2e_*` suite this really executes in CI. `count_min_sketch` is correctly added to the random-seed normalization list. One honest gap the commit surfaces but doesn't close (non-blocking follow-up): `uuid` and the 8 `tuple_*`/datasketches builders are excluded from the golden because "reference adds a seed/default arg ours omits" — i.e. our proto for those specific builders diverges from reference by a missing trailing arg. Worth a separate fix so they too can be golden-pinned, but it's pre-existing and out of scope here. **Still open from earlier passes** (this commit didn't address them): - **A (LOW):** `PyDataFrameWriterV2::option`/`options` (pyspark-rs `dataframe.rs:1300`,`:1312`) still bypass `coerce_option_value` — `writeTo(...).options(x=None)` sends `"None"`. - **B (MEDIUM, test infra):** the `e2e_*` suite still doesn't run in CI (`SPARK_REMOTE` unset), and the `RightOuter`/`FullOuter`/`LeftSemi`/`LeftAnti` joins + `local_relation`/`cached_remote_relation` still lack a server-free serialization guard after the golden→behavioral swap. (The serverless `udf_builders_client_side` test is also still gated behind `should_run()` for no reason.) No blocker. Verdict unchanged: good to merge once A/B are addressed or consciously deferred. 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]
