HyukjinKwon commented on PR #50:
URL:
https://github.com/apache/spark-connect-rust/pull/50#issuecomment-5403248123
Addressed the assisted-review findings on this PR. Commits on
`HyukjinKwon:split-rust-core`.
**1. [High] Streaming listeners never delivered live events** — fixed.
`stream_listener_events()` (the accumulate-into-a-`Vec`-until-the-server-closes
stub) is replaced by an incremental `ListenerEventStream` (an `Iterator<Item =
Result<(i32, String)>>`) built from a `StreamingQueryListenerBusCommand {
add_listener_bus_listener: true, .. }` execute stream — each `next()` pulls one
`ExecutePlanResponse` and yields its events, so callbacks fire live. The dead
command locals are gone. (Python side + the GIL release for the daemon thread
are on #51.)
**2. [Medium] `get_message()` double-prefixed the error class** — fixed.
`get_message()` now returns `self.message()` (single `[CLASS]` prefix), and the
unit test that locked in the doubled string is corrected.
**3. [Medium] WASM `python_ver` hardcoded `"3.11"`** — fixed.
`default_python_ver()` now runs the packer's own interpreter
(`$SPARK_CONNECT_PYTHON`/`$PYSPARK_PYTHON` else `python3`) and parses
`MAJOR.MINOR`, so the reported version matches the interpreter that actually
cloudpickles; falls back gracefully if the query fails, `with_python_ver()`
still overrides. Added a parse-helper test.
**4. [Low] `UnresolvedRegex.plan_id`** — fixed: `expression.rs` now uses
`plan_id: None`, consistent with the `col_regex` path in `plan.rs`.
**5. [Low] `UInt64Array` → `i64` silent wrap** — fixed: now a checked
`i64::try_from(...)` that errors on out-of-range instead of truncating.
**Also found while checking the audit's blind spot (not in the report):**
- `grouping_sets([[a,b],[a]])` was flattening all columns into a plain
`groupBy` and dropping the set structure. Now it emits
`GROUP_TYPE_GROUPING_SETS` with the `grouping_sets` field populated (added
`AggregateGroupType::GroupingSets`, `aggregate_with_grouping_sets`,
`GroupedData::new_grouping_sets`), with a golden test asserting the sets
survive.
- The listener stub's wording ("simplified implementation … would stream
events") slipped past `audit_no_stubs.sh`, so I broadened the pattern list to
catch that class.
--
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]
