viirya commented on PR #54:
URL:
https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5419297669
Re-reviewed after the updates. Every item I raised has been addressed, and I
verified each one in the code rather than taking the commit messages at face
value. Two of the fixes went further than I suggested, and one of them exposed
a mistake in my own earlier review that I want to correct explicitly. One new
finding at the end.
## The deselection bug — and a correction to my last comment
Commit `bc33579` is the important one here. The manifest deselection **never
matched anything**: pytest was handed an absolute test path while it emits node
ids relative to its rootdir, so every `--deselect <abspath>::Class::test`
silently matched nothing and the whole 65-entry manifest was void.
I need to correct myself on this. My last comment said the matching was
"exact, not substring — so a loose match can't silently swallow neighbouring
tests," and that a malformed entry "matches nothing, meaning the test runs and
fails loudly. That fails safe." The first half was right about the mechanism
and the wrong conclusion to draw: *every* entry was matching nothing, so the
property I described as a safety net was in fact the bug. I checked the
matching logic in isolation and never asked whether it matched in practice —
running the gate, or even just noting that a 65-entry manifest ought to change
the failure count, would have caught it.
The knock-on effect is the interesting part: once deselection worked and the
missing `test_support/sql` fixtures were vendored, most of the manifest turned
out to be stale. It's now **2 entries**, both with specific, checkable reasons
(`testcat` needing `InMemoryCatalog`, a test-only class absent from the
distribution) rather than the uniform `# reference also fails (environmental)`
boilerplate. So the gate now genuinely exercises 63 tests it was previously
reporting on but not really running. That's a much bigger improvement in gate
strength than anything I asked for.
## Confirmed fixed
- **Retries are scoped.** `FLAKY_FILES` (`run_official_tests.py:43-52`) is
an explicit 7-file set — the streaming/listener/observation/transformWithState
files — and `retries = args.retries if tf.name in FLAKY_FILES else 0` (line
228). Everything else gets zero retries, so a non-deterministic bug outside
those files fails the gate on first occurrence.
- **Drift check added, and non-blocking as suggested.** Lines 263-288 re-run
the skiplisted tests selecting only them, and report any that now pass without
failing the gate. The comment states the rationale (the manifest "can only rot
toward less coverage"). This is what caught the 29/31 stale entries.
- **`stat.rs` (4) and `conf.rs` (5) now release the GIL.**
- **Sort uses the generated enum constants** — `SortDirection::Ascending as
i32` and the `NullOrdering` import, no more bare `1i32`.
- **Map keys stringify to their scalar form.** A dedicated
`map_key_to_string` (`dataframe.rs:2102`) with a comment naming the exact case:
a `map<int,string>` key must be `"1"`, never `"Integer(1)"`.
- **`rust_coverage.sh` no longer swallows failures** — the `|| true` is
gone, with a comment distinguishing "exits 0 on per-op gaps" from "cannot run
at all."
- **The coverage-gate claim is corrected.** The description now carries an
explicit "Coverage status (no gate enforced yet)" section saying there is
**no** `rust-coverage` CI job and that the script is local tooling. That's the
honest framing.
- **Branch is rebased** onto master, so #53 is included.
## The test-quality response went further than I asked
I suggested adding value assertions to `e2e_wrapper.py`. What landed is more
than that:
- `ckv(label, fn, expected)` asserts results, and its docstring names the
failure mode it exists for (a nullary `F.struct()` returning an empty struct
and throwing nothing). 14 call sites so far against 179 `ck`, so most of the
wrapper is still exception-only — reasonable, since not every op has a cheap
expected value, and `ck`'s own docstring now points readers at `ckv`.
- More significantly, the three assertion-free `*_coverage_golden.rs`
snapshot files were **removed** and replaced with behavioral tests that assert
real results against a live server: `e2e_functions.rs` (24 assertions),
`e2e_columns.rs` (37), `e2e_dataframe.rs` (25), `types_behavior.rs` (55).
Swapping ~1,200 lines of "does not panic" for 141 real assertions is the right
trade.
- And they earned their keep immediately — `600c8e4` and `37c442c` fix
`select_expr`, `to_json`, and `DataType::String.simpleString` as bugs the
behavioral tests found. That's the difference between a coverage vehicle and a
test suite.
Separately, `ca32e8c` is a good catch that wasn't in my review at all:
`StreamingQuery::_execute_command` and the manager equivalent sent their
command and discarded the response stream, so `status()`/`isActive()` failed
and `explain`/`exception`/`lastProgress`/`recentProgress`/`awaitTermination`
all silently saw empty results. Same "result discarded" shape as the
plan-builder family.
## New: the behavioral e2e tests don't run in CI
The four new behavioral files all gate on `SPARK_REMOTE`:
```rust
fn should_run() -> bool { std::env::var("SPARK_REMOTE").is_ok() }
...
if !should_run() { return; }
```
No workflow sets `SPARK_REMOTE` — I grepped `.github/workflows/` and there
are no matches — so all 141 assertions currently no-op in CI. Worse, because
the guard is an early `return` rather than `#[ignore]`, they report as
**passed** rather than skipped, so the test count looks healthy while nothing
is being checked.
This matters more than usual given the context: these tests exist precisely
because the proto-snapshot tests they replaced couldn't catch silently-wrong
values, and they've already found three real bugs locally. As written they
protect a developer who happens to have a server running, and nobody else — a
regression in `select_expr` or `to_json` would land green.
Two options, and the choice depends on how much CI time you want to spend.
The parity job already stands up a Spark 4.2.0 Connect server, so pointing
`SPARK_REMOTE` at it and running `cargo test -p apache-spark-connect --test
e2e_functions --test e2e_columns --test e2e_dataframe --test types_behavior` in
that job would be nearly free. If you'd rather not couple them, switching the
guard to `#[ignore]` (or having `should_run()` panic when a
`REQUIRE_SPARK_REMOTE` flag is set) would at least make the skip visible
instead of looking like a pass.
## Smaller notes
`types_behavior.rs` has 55 assertions and does not appear to need a server
for most of them (proto/JSON round-trips, `StructType` helpers) — worth
confirming those run unconditionally, since they'd be the portion that can
guard CI today without any server plumbing.
## What I verified
I read the updated scripts, workflows, and the changed core/binding files,
and grepped the workflow tree for `SPARK_REMOTE`. I did not run the parity
gate, the coverage script, or the e2e tests against a live server, so the
claims about which tests now pass (the 29/31 stale entries, 32/32 datasources)
I'm taking from the commit messages rather than independent confirmation.
--
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]
