viirya commented on PR #54:
URL:
https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5420369794
Third pass. My open item is fixed and it paid off immediately. I ran the
suite this round rather than only reading it — 329 tests pass, 0 failures on
the default (server-free) members. One residual robustness item at the end;
nothing I'd consider blocking.
## The e2e-in-CI fix landed, and found a bug within one commit
`SPARK_REMOTE: sc://localhost:15002` is now set in the `connect-parity` job
(`build_python_connect.yml:180`), reusing the Spark 4.2.0 server already
standing up there and running five e2e targets with `--test-threads=1` since
they share a session. Reusing the existing server rather than adding a job is
the right call, and the comment explains why the step exists.
The payoff is `3559267`: with those assertions actually executing,
`e2e_coverage::catalog_surface` caught that the catalog list ops (`listTables`,
`listColumns`, `getDatabase`, …) eagerly executed and then (a) errored with
"Catalog operation returned no rows" whenever the result was *legitimately*
empty — `listTables` on a database with no tables — and (b) rebuilt the
DataFrame as a `LocalRelation` with `data: None`, discarding the real rows and
coercing every column to String.
The fix is the right shape: represent the op as a relation
(`LogicalPlan::Catalog` → `RelType::Catalog`) and return it lazily, so the
server's actual schema and rows flow through `.collect()`. That fixes the empty
case and the schema/data loss together, rather than special-casing the empty
result. `RelType::Catalog` is already covered by `catalog_golden.rs`, so the
new plan variant isn't untested.
## Restoring the server-free net was a good catch I'd missed
`plan_serialization.rs` is an unprompted addition, and it closes a gap my
own review opened: swapping the assertion-free `*_coverage_golden.rs` snapshots
for live-server behavioral tests removed the coverage that ran *without* a
server. The new file puts that back — 6 assertions, zero server gates, so it
runs in the server-less job — and covers all seven `JoinType` variants mapped
to their proto enums, including `RightOuter`/`FullOuter`/`LeftSemi`/`LeftAnti`.
Those are exactly the ones where an off-by-one in an enum mapping produces
silently wrong joins rather than an error, so pinning them server-free is worth
more than the line count suggests.
## Also confirmed
- **Golden set extended 440 → 508** byte-for-byte against the reference
client, after re-capturing (518 available vs the stale 440).
- **`uuid` and `tuple_sketch_agg_{double,integer}`** now materialize the
trailing default args the reference emits, so they rejoin the byte-for-byte set
instead of being excluded. `count_min_sketch` joins the random-seed
normalization.
- **`DataFrameWriterV2.option`/`options`** now route through
`coerce_option_value` like the V1 writer, so `writeTo(...).options(x=None)` no
longer sends the string `"None"` and `option(k, True)` no longer sends `"True"`.
- **`types_behavior.rs`** has no server gate, so its 55 assertions run
unconditionally in the server-less job — which was the smaller note from my
last comment.
## Residual: the silent-pass guard is now a latent trap rather than a live
bug
The CI wiring is correct, so the coverage is real today. But the guard shape
that caused the original problem is unchanged:
```rust
fn should_run() -> bool { std::env::var("SPARK_REMOTE").is_ok() }
...
if !should_run() { return; }
```
I checked what that reports with no server configured:
```
$ cargo test -p apache-spark-connect --test e2e_functions
running 3 tests
test result: ok. 3 passed; 0 failed ... finished in 0.00s
```
Three tests report **passed** in 0.00 s while asserting nothing.
Today that's harmless because the CI step exists. The concern is that
nothing detects it silently going away: if the step is renamed or reordered, or
the server fails to come up and the step's failure is tolerated, these tests
revert to reporting green while checking nothing — and the only signal would be
a bug like the catalog one shipping unnoticed. Given the whole point of these
tests is catching silently-wrong values, a guard that fails silently is an
unfortunate pairing.
Cheapest fix I can see: have `should_run()` panic (or `assert!`) when a
`REQUIRE_SPARK_REMOTE=1` env var is set, and export that in the CI step. A
missing server then fails loudly exactly where it matters, while local `cargo
test` keeps skipping freely. `#[ignore]` would also make the skip visible
rather than a pass, though it changes how you invoke them locally.
## New: an unreachable server makes the e2e tests hang for ~13 minutes, not
fail
I went looking for the other half of the guard behaviour — what happens when
`SPARK_REMOTE` *is* set but nothing is listening — and it isn't a quick failure:
```
$ SPARK_REMOTE=sc://127.0.0.1:59999 cargo test -p apache-spark-connect
--test e2e_functions
running 3 tests
test aggregate_functions has been running for over 60 seconds
test conditional_and_collection_functions has been running for over 60
seconds
test math_and_string_functions has been running for over 60 seconds
```
They were still running when I gave up at four minutes. The cause is the
retry wiring this PR adds, working as designed: a refused connection surfaces
as `UNAVAILABLE`, `RetryPolicy::can_retry` treats that as retryable, and
`with_retry` (which `analyze_plan` and `execute_plan_reattachable` both go
through) then burns the default budget — `max_retries: 15`,
`initial_backoff_ms: 1000`, `backoff_multiplier: 2.0`, `max_backoff_ms: 64000`.
That's ~10.7 min of backoff plus 16 × the 10 s `READY_TIMEOUT`, so roughly **13
minutes per operation** before the error surfaces.
This isn't a defect in the retry policy — it mirrors the reference client,
and for a genuinely flaky server it is the behaviour you want. But it interacts
badly with the new CI step in a specific way: if the Spark server fails to come
up in `connect-parity`, the e2e step won't fail fast with "connection refused";
it will sit there retrying until the job or step timeout kills it, and the log
will show tests "running for over 60 seconds" rather than the actual cause.
Worth either capping retries for the e2e step (an env override, or a shorter
policy when `SPARK_REMOTE` is explicitly set) or making sure that step carries
a timeout tight enough to fail with a legible message. Same consideration
applies to anyone running these locally against a server that isn't up — the
13-minute hang reads as a broken test rather than a missing server.
## What I verified
I ran `cargo test` on the default members (329 passed, 0 failed), and ran
`e2e_functions` both without `SPARK_REMOTE` (3 tests report passed in 0.00 s)
and with it pointed at a closed port (hangs past four minutes; the ~13 min
figure is computed from the default `RetryPolicy` constants, not measured to
completion). I did not run the parity gate, the coverage script, or the e2e
tests against a live server, so the claims about the catalog fix and the
38-test e2e suite passing against 4.2.0 I'm taking from the commit messages.
--
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]
