HyukjinKwon commented on PR #54:
URL:
https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5418883835
Thanks — all 10 findings addressed. Two commits: parity-gate correctness,
then the functional/fidelity fixes.
**Must-fix**
- **#1 TIMESTAMP `createDataFrame` regression** — `build_arrow_array` now
takes the target field type and tags the timestamp array's zone to match the
schema (UTC for TIMESTAMP/LTZ, none for TIMESTAMP_NTZ), so
`RecordBatch::try_new` no longer rejects it. Covered by a new
`e2e_coverage::create_dataframe_typed_schemas` (TIMESTAMP + TIMESTAMP_NTZ +
Float-from-int).
- **#2 gate scores `ok` with no summary line** — `run_ours` now gates on
pytest's **exit code** (0 ok, 5 = fully-deselected/no-tests ok, anything else
fail) via a shared `is_bad()` used by the retry loop, the failure decision, and
the drift check. A plugin-load/collection/crash exit is no longer mistaken for
green.
- **#3 plural `"errors"`** — regex is now `errors?` (and the returncode
check covers it regardless).
**Fidelity**
- **#4** `approxQuantile` returns `list[float]` (collects the server's
array-of-arrays, returns the inner list).
- **#5** `persist(level)` honors the requested `StorageLevel` (default →
`MEMORY_AND_DISK_DESER`).
- **#6** `repartition("col")` / `repartition(col)` column-first form works
(`num_partitions <= 0` serializes as unset → server default).
- **#7** new `coerce_option_value` across all reader/writer/streaming/conf
`option(s)`/`set`: `None` leaves the option unset, booleans lowercase to
`"true"`/`"false"`.
- **#8** `hint(name, 3)` accepts int params and sends an `Integer` (int32)
literal (a `Long` was rejected by REBALANCE).
- **#10** `coerce_value` widens `Long`/`Integer` → `Float` for a FLOAT
`createDataFrame` column.
**#9** (`__getattr__` returns a Column for any name) left as the noted
deliberate trade-off (validating would need a schema round-trip per attribute
access).
On the coverage note: agreed — the parity suite + Rust golden/behavioral
tests are the real gate, and `e2e_wrapper`'s `ckv` assertions are a local
coverage vehicle. Separately, I've been replacing proto-snapshot coverage tests
with **behavioral** e2e that assert results against reference semantics (both
Rust and the Python drop-in) — which is how #1/#4-#8 above and several earlier
bugs (`select_expr`, `to_json`, `String.simpleString`) were caught.
--
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]
