HyukjinKwon commented on PR #50:
URL:
https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5403048057
**Assisted code review** (findings verified against the diff; generated
proto and golden fixtures skipped, `functions.rs`/`types.rs` cross-checked
against the golden tests and came back clean).
**1. [High] Streaming query listeners never deliver live events** —
`crates/spark-connect/src/streaming.rs:869`
`stream_listener_events()` loops `while let Some(resp) =
response_stream.message()` accumulating into a `Vec` and only returns
`Ok(events)` once the server *closes* the stream (`:906`). The Python handler
(`python/pyspark/sql/streaming/query.py:_query_event_handler`) calls it
**once** and iterates the result. For a live streaming query the listener-bus
stream stays open, so the daemon thread blocks indefinitely and **no
`onQueryProgress`/`onQueryIdle`/`onQueryTerminated` callback ever fires** in
real time. The in-code comment ("simplified implementation - the actual
implementation would stream events") confirms this is a stub. Secondary: the
method builds several unused proto locals (`listener_bus_cmd`, `listener_cmd`,
the first `streaming_cmd`) — dead code; and `remove()` (`query.py`) does
`join(timeout=5)` then drops the reference, leaving the blocked daemon thread
alive.
**2. [Medium] `get_message()` double-prefixes the error class** —
`crates/spark-connect-core/src/error.rs:407`
`message()` (`:296`) already returns `"[ERROR_CLASS] rendered"`.
`get_message()` prepends the class again, yielding `"[CANNOT_BE_NONE]
[CANNOT_BE_NONE] Argument `x` cannot be None."`. This diverges from the
`PySparkException.getMessage()` contract it documents mirroring. The unit test
at `:1029` asserts the doubled string, so it locks in the wrong output instead
of catching it. Fix: `get_message()` should return `self.message()`.
**3. [Medium] WASM UDF `python_ver` hardcoded to `"3.11"`** —
`crates/spark-connect/src/wasm_udf.rs:397` (used at `:291`)
The UDF command reports Python `"3.11"` regardless of the packer's actual
interpreter. Spark's worker enforces driver/worker Python minor-version parity,
so this raises a version-mismatch error whenever executors run a different
minor version (e.g. 3.10/3.12). The default should be derived from the packer's
Python; `with_python_ver()` exists as an override but the default is wrong.
**4. [Low] `UnresolvedRegex.plan_id` inconsistency** —
`crates/spark-connect/src/expression.rs:87`
This path hardcodes `plan_id: Some(0)`, while the actual `col_regex` path
(`plan.rs:806`) correctly uses `None`. The golden harness masks the difference
by normalizing `plan_id` to `None` (`functions_golden.rs:28`,
`plans_golden.rs:221`, …). Harmless today (the public API uses the `None`
path), but latent and hidden by the test normalization.
**5. [Low] `UInt64Array` → `i64` silent wrap** —
`crates/spark-connect/src/dataframe.rs:2082`
`arr.value(index) as i64` wraps values > `i64::MAX`. Not reachable from real
Spark (Spark SQL has no unsigned types), but a latent truncation if a `UInt64`
array is ever decoded.
--
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]
