HyukjinKwon commented on PR #54: URL: https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5425345934
<!-- ai-code-review --> ## Code review — manual correctness pass **Note on scope:** the automated `spark-dev` scanner pipeline could not complete on the reviewer's host (macOS): the pager's scratch-root allowlist compares against a literal `/tmp`, but `Path.resolve()` rewrites `/tmp` → `/private/tmp`, so it never matches, and the sandboxed scanner subagents also can't write under `/tmp`. The `text_quality` scanner did complete (**no findings**); the `contract_claim_verifier`, `link_verifier`, and `local_efficiency` scanners could not run. So this is a best-effort manual review of the core behavioral changes, read directly from the diff — not the plugin's certified sign-off. ### Headline claims verified against the diff | Claim | Verdict | |---|---| | 16 variadic fns took zero args (`struct`/`array`/`coalesce`/…) | ✅ now `Vec<Column>`; dispatch passes `args.clone()` | | Streaming command responses discarded | ✅ `_execute_command`/`_execute_manager_command` drain via `execute_command_collect` and return the first typed result | | `Sort` dropped bare columns | ✅ a bare (non-`SortOrder`) column now defaults to asc/nulls-first instead of yielding an empty, invalid `Sort` | | Two Arrow decoders unified (11→36 types) | ✅ `catalog.rs` delegates to the single `dataframe::arrow_value_at`, now covering Decimal256/FixedSizeBinary/Time/Interval | | Non-string map keys render scalar value | ✅ new `map_key_to_string` — a `map<int,…>` key is `"1"`, not `"Integer(1)"` | | Every `DataType` picklable, recursive parse | ✅ each PyO3 type has `__reduce__` → `_parse_datatype_json_string`; core parse handles nested object forms | | Struct metadata stored raw (no re-quoting) | ✅ `parse_json_object` stores `v.as_str()` raw, JSON-text fallback for non-strings | | `is_stopped` real backing | ✅ `Arc<AtomicBool>` shared across `clone()` | | WASM UDF SQL-registerable | ✅ `spark.udf().register(name, &udf)` sends `RegisterFunction` | Additional correct fixes beyond the description: a genuine **UTF-8 bug fix** in `types.rs` (`chars().enumerate()` → `char_indices()`, since the index is used as a byte offset for slicing); `explain_mode`; `semanticHash`/`sameSemantics` moved to real server `AnalyzePlan`; `toJSON` via server `to_json(struct(*))`; `createDataFrame` Arrow type-coercion + timezone handling (LTZ=UTC vs NTZ=None); Hint param `Long`→`int32`; `RepartitionByExpression` optional count. ### One informational note (not blocking) `functions::uuid()` now emits a **fixed** seed literal. This is deliberate (documented, for golden-test determinism, mirroring `rand`/`randn`), but it diverges subtly from reference pyspark, where each `uuid()`/`rand()` call draws a fresh random seed — so `df.select(uuid(), uuid())` yields two identical columns here versus two independent ones in pyspark. Harmless for the golden suite; worth a note if strict per-call independence ever matters. **No blocking correctness issues found in the core changes.** For a certified automated pass, re-run the review on Linux (or a host where `/tmp` isn't a symlink). -- 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]
