HyukjinKwon commented on PR #54: URL: https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5417397265
Thanks @viirya — this is a thorough review and every point was actionable. All addressed on the current head (rebased onto master, so #53's docs are now included). Point by point: **Parity gate** - *Retries applied to every file* — fixed. Retries are now scoped to an explicit `FLAKY_FILES` set (streaming-listener/observation, the genuinely event-driven ones); every other file gets **zero** retries, so a non-deterministic bug there surfaces on its first occurrence rather than needing to reproduce 3×. - *No "unexpectedly passing" detection* — added a post-run **drift check** that re-runs the skiplisted tests (a new select-only mode in the plugin) and **reports** (never fails on) any that now pass, so the list can't silently rot. - *Uniform boilerplate reasons* — regenerated the manifest with the **captured error class per entry** (mostly `[UNSUPPORTED_TIME_TYPE]` on the pinned 4.2.0 server build, `testcat` InMemoryCatalog, single-node `TaskResourceProfiles`) and dropped the entries that now pass, leaving 31 genuine, auditable failures. Separately, I found the deselection itself never matched in CI (pytest rootdir differs between a dist and a source clone), which had voided the whole manifest — deselection now happens in the plugin by node-id **suffix** match, which is rootdir-independent. **Coverage gate doesn't exist** — corrected the PR description; there is no enforced gate (deferred). Also removed the two `|| true` in `rust_coverage.sh` so a dead Python suite fails loudly instead of reporting silently-low coverage. **e2e_wrapper proves absence of exceptions, not correctness** — added `ckv()` asserting `fn() == expected` and pinned actual values for the wrong-but-non-throwing-prone ops, especially the variadic functions (struct/array/coalesce/concat/create_map now assert populated results). 188/188 incl. the assertions. **Smaller items** — all fixed: GIL now `py.detach()`ed in `stat.rs` (corr/cov) and `conf.rs`; non-string map keys format the scalar (`"1"`, not `"Integer(1)"`); Sort uses the generated `SortDirection`/`NullOrdering` enum constants instead of 1/2. And the branch is rebased onto master. Also, per an offline question: `to_datafusion`/`to_polars`/`to_arrow` were compiled but never behaviorally tested — added deterministic conversion unit tests (synthetic RecordBatches, in the `--features datafusion,polars` job) plus a server-gated `to_arrow` e2e test. -- 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]
