HyukjinKwon commented on PR #54:
URL:
https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5419042912
<!-- ai-code-review -->
## Re-review at `9a34c8a` (after the fixes)
Took a fresh, thorough pass over the new head — verified every fix against
the code (again manually; the `spark-dev page` harness still can't read its
`/tmp` scratch on macOS). **All three must-fix items from the first pass are
correctly and completely resolved.** Details, then two small new items this
pass surfaced.
### Verified fixed ✅
- **#1 TIMESTAMP `createDataFrame` regression** — `build_arrow_array` now
threads the target field type and applies `.with_timezone("UTC")` for LTZ /
none for NTZ (session.rs:843-855), so the array type matches the `Timestamp(us,
Some("UTC"))` schema. Correct.
- **#2 gate scores `ok` on a no-summary run** — `run_ours` now records
`counts["rc"]` and a shared `is_bad()` gates on pytest's exit code across the
retry loop, the failure decision, and the drift check; `rc=5` (no tests
collected) is treated as ok, and a no-summary nonzero exit is surfaced as
`rc=N`. A broken extension/plugin load now fails the gate. Correct.
- **#3 plural `"errors"` regex** — now `(\d+) errors?\b`, and subsumed by
the returncode check anyway. Correct.
- **#4 `approxQuantile`** returns `list[float]` (collects the inner quantile
list, `[]` on empty, no panic). **#5 `persist(level)`** honors the StorageLevel
(default → `MEMORY_AND_DISK_DESER`). **#6 `repartition("col")`** column-first
form works and `num_partitions <= 0` serializes as unset (plan.rs:665). **#8
`hint(name, 3)`** accepts ints and sends an `Integer`/int32 literal
(plan.rs:748). **#10 `coerce_value`** widens Long/Integer → Float. All correct.
- The new production fixes also check out: **`selectExpr`** now uses
`functions::expr` (server-parsed SQL, one expr per string), **`toJSON`**
projects `to_json(struct(*))` server-side, and **`DataType.simpleString`**
returns `"string"` for the default/UTF8_BINARY collation. Correct.
### New this pass
**A. LOW — the option-coercion fix (#7) missed `DataFrameWriterV2`.**
`coerce_option_value` (None → unset, bool → lowercase) was wired into the
reader, the V1 writer, streaming reader/writer, and `conf.set` — but
`crates/pyspark-rs/src/dataframe.rs:1300` `PyDataFrameWriterV2::option(value:
&str)` and `:1312` `options` still use raw `v.str()`. So
`df.writeTo("t").options(mergeSchema=None)` inserts the literal `"None"` and
`options(overwrite=True)` sends `"True"`, same bug class as #7. Uncommon path
(V2 writer), but it's an incomplete fix.
**B. MEDIUM (test infra) — the new `e2e_*` tests don't run in CI, and the
golden→behavioral swap dropped a few server-free serialization guards.**
- `e2e_functions.rs` / `e2e_columns.rs` / `e2e_dataframe.rs` and the
server-gated tests in `e2e_coverage.rs` all self-gate on `should_run()` = `env
SPARK_REMOTE is_ok()`. **No workflow sets `SPARK_REMOTE`** — CI uses
`SPARK_CONNECT_TESTING_REMOTE` for the Python parity gate, and the `cargo test`
invocations (`rust.yml`, the `rust-checks` job) run with no server. So these
tests return immediately and assert nothing in CI; their value assertions
(which are genuinely good — `md5("abc")=…`, `element_at(arr,1)=3`, etc.) only
run locally. (`types_behavior.rs` is *not* gated and does run in CI — good.)
- The swap deleted `plan_coverage_golden.rs::every_plan_builder_serializes`,
a server-free completeness net. Most of its surface survives
(`functions_golden.rs` 440 cases, `plans_golden.rs` 46, and `grouping_sets` is
still guarded by `plan_arg_regression_golden.rs:443`), but I couldn't find a
surviving CI-running serialization test for the `RightOuter` / `FullOuter` /
`LeftSemi` / `LeftAnti` join variants or `local_relation` /
`cached_remote_relation` — their only remaining coverage is the CI-skipped
`e2e_*` tests (joins are still exercised indirectly by the live Python parity
gate).
Neither A nor B is a functional/production bug. Two cheap closes for B: set
`SPARK_REMOTE=sc://localhost:15002` in the `connect-parity` job and run `cargo
test` there against the already-running server, and/or keep a server-free
serialization guard for the dropped plan builders.
### Out of scope (pre-existing, untouched by this PR)
`functions::cast(_col, to_col) -> to_col` (functions.rs:96) ignores its
input column and returns the target as-is — a no-op that `audit_no_stubs.sh`
doesn't catch (it compiles and returns a value). Introduced in the merged core
commit, not this PR, and there's no `pyspark.sql.functions.cast` in reference
PySpark for it to mirror — worth a separate follow-up.
---
**Verdict:** the blockers are resolved; what remains is one minor fidelity
gap (A) and test-infra hygiene (B). 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]
