viirya commented on PR #54:
URL:
https://github.com/apache/spark-connect-rust/pull/54#issuecomment-5415125899
Reviewed the whole change — core semantics, the PyO3/skin layer, and the CI
restructuring. A lot of this is genuinely good work, and two of my earlier
findings are properly fixed. My concerns are concentrated in the parity gate,
which I think is meaningfully weaker than it looks, and in a few smaller items
below.
## First, what's clearly right
Worth stating because it's the bulk of the diff:
- **The variadic fix is a real, severe bug fix.** On current `master`,
`array()`, `coalesce()`, `concat()`, `r#struct()`, `hash()`, `create_map()`,
`elt()`, `stack()`, `xxhash64()`, `arrays_zip()` and friends take **zero
arguments** — so `F.struct(a, b, c)` really did produce an empty struct. 16
functions across two commits, and the same bug class as the `dropna`/`hint`
family from the earlier reviews. Making them `Vec<Column>` and adding a
variadic category to the dispatch generator is the right structural fix rather
than another one-off.
- **The `Sort` fix is correct.** A bare column previously fell through both
branches and was dropped, leaving an empty `order` and an invalid plan. The new
default of `direction = 1` / `null_ordering = 1` maps to
`SORT_DIRECTION_ASCENDING` / `SORT_NULLS_FIRST`, which matches reference
`asc()`. (Minor: those two magic numbers would read better as the generated
enum constants — `SortDirection::Ascending as i32` — since a future proto
reorder would silently change meaning.)
- **The Arrow decoder unification is a clean win with no regression.** I
checked every type the deleted `catalog.rs` decoder handled — all 11 are
present in the surviving `dataframe::arrow_value_at`, and coverage goes from 11
to 36 array types. Decimal128/256 preserve precision/scale as strings rather
than going through `f64`, which is the correct choice. Deleting a duplicate
decoder rather than patching both is the right call.
- **`session.rs` GIL is fixed.** This was my open item from the last round:
it now has 10 `py.detach()` calls covering `get_or_create`, `stop`, `version`,
`interrupt_all`/`interrupt_tag`/`interrupt_operation`, `add_artifact(s)`,
`build_resource_profile`, and `create_dataframe`. Thanks for closing that.
- The API-fidelity work checks out where I sampled it: `head()` correctly
returns `Row | None` vs `List[Row]` on the `n` overload, `fillna` dispatches
dict vs scalar properly, `repartition(numPartitions, *cols)` matches,
`Row(*args)` yields `_1.._n` while `Row(**kwargs)` yields named fields and
mixing them raises, and the exception types
(`IndexError`/`KeyError`/`AttributeError`) match reference Row semantics.
## The parity gate is weaker than the description implies
This is my main concern, and it's about the gate's design rather than any
single line.
**Retries are applied to every file, not just the flaky ones.** The
description says there's "a per-file retry for the timing-sensitive
streaming-listener/observation tests," and the `--retries` help text gives a
well-reasoned justification scoped to those tests. But the implementation in
`work()` has no file filter at all — the `while` loop at
`run_official_tests.py:165-170` re-runs *any* file that reports a failure, and
CI invokes the script without `--retries`, so the default of 2 (three attempts
total) applies to all 84 files.
The consequence: a client bug that reproduces intermittently — a race in
reattach, a timing-dependent streaming path, anything resource-sensitive — now
needs to fail **three times in a row** to be reported. A bug that fails 50% of
the time has a ~12% chance of being caught per run. The help text's claim that
"a real regression still fails every attempt" holds only for deterministic
regressions; it's exactly the non-deterministic ones this masks. Scoping the
retry to the handful of genuinely event-driven files (an explicit list, or a
marker) would keep the intended benefit without blanket-hiding flaky failures
everywhere else.
**Nothing detects a skiplist entry that starts passing.**
`parity_known_failures.txt` has 65 entries and there's no "unexpectedly
passing" check anywhere in `run_official_tests.py` or `gen_parity_skiplist.py`,
and the generator only runs manually on a version bump. So the list can only
ever rot in the direction of less coverage: once a test is in it, nothing
re-examines it, and a genuine future regression in a skiplisted area is
permanently invisible. A cheap fix is to run the skiplisted tests separately
(not deselected) and report — not fail — any that now pass, so drift surfaces
without blocking.
**The reasons are uniform boilerplate.** All 65 entries carry the identical
comment `# reference also fails (environmental)`. I want to be fair here: these
were *generated* by actually running the reference client, so "reference also
fails" is an empirical observation, not a guess — and several are plausible in
a single-node pure-Connect environment (`test_udf_with_input_file_name` with no
real input file, the `foreach_batch` family needing a working Python-worker
callback path). But because no entry records *why* the reference failed, nobody
can later distinguish "environmental, still true" from "was a real gap, now
fixed" from "was misclassified." That's what makes the staleness problem above
unfixable in practice. Capturing the reference's actual error class per entry
when generating would cost little and make the manifest auditable.
On the mechanics, two things I checked that are **fine**: entries are
matched via pytest `--deselect` with full node IDs
(`run_official_tests.py:104`), which is exact, not substring — so a loose match
can't silently swallow neighbouring tests. And a malformed entry matches
nothing, meaning the test runs and fails loudly. That fails safe, which is the
right direction.
## The coverage gate described in the PR doesn't exist
The description says "a new `rust-coverage` CI job fails under
`COVERAGE_MIN` (90%)", and the testing section lists "the coverage gate (≥90%)"
as having run on this PR. Neither is true of the current branch: no workflow
references `rust_coverage.sh`, `COVERAGE_MIN`, or a coverage job. Commit
`3c05ab9` ("Keep coverage as tooling; defer the gating CI job") deliberately
removed it, so the description is describing an earlier state of the branch.
The script itself is fine as local tooling, but two `|| true` at
`rust_coverage.sh:81,85` swallow failures from the e2e and official-test runs,
so a run where the Python suite dies still produces a report — just with
silently lower coverage. If the gate is later turned on, that needs to fail
instead. I'd just correct the description now so reviewers aren't told a gate
is enforcing something it isn't.
## `e2e_wrapper.py` proves absence of exceptions, not correctness
The 174-operation exercise is a reasonable coverage vehicle, and driving the
real drop-in API is exactly the gap the transport-injection gate leaves. But
`ck()` (line 46) only catches exceptions:
```python
def ck(label, fn):
try:
fn()
_ok.append(label)
except Exception as e:
_fail.append(...)
```
No result is ever asserted. `ck("session.range", lambda:
spark.range(3).collect())` passes whether `collect()` returns 3 correct rows, 0
rows, or garbage. That matters specifically here, because **the bug class this
PR fixes is silently-wrong values, not exceptions** — a nullary `F.struct()`
returning an empty struct throws nothing, so this harness would have reported
174/174 green while the bug was live. Asserting even coarse expectations for a
subset (row counts, a known value or two) would turn it from a smoke test into
something that can catch a regression.
The same caveat applies to the three new `*_coverage_golden.rs` files,
though they're honest about it — the headers say "this only guards that no
builder panics or is left untested," and `functions_coverage_golden.rs` is 949
lines with one assertion. That's a legitimate purpose (it's the coverage
vehicle), and real parity does live in `functions_golden.rs` plus
`tests/golden/functions.jsonl`, which I confirmed contains `struct`, `hash`,
`array`, `coalesce`, `concat`, `elt`, `create_map`, and `arrays_zip`. Worth
being aware that ~1,200 lines of new test code deliberately assert almost
nothing, so a coverage number derived from them overstates how much behaviour
is actually pinned.
## Smaller items
**GIL held in the two new binding files.** `stat.rs` and `conf.rs` have zero
`py.detach()`. In `stat.rs`, `corr` and `cov` call `df.scalar()?`, which
executes a plan and round-trips to the server, so those hold the GIL for the
duration (`crosstab`/`freq_items` return DataFrames and are lazy, so they're
fine). In `conf.rs`, `RuntimeConf::set`/`get` reach
`block_on(self.client.set_config(...))` / `get_configs(...)` in the core. Same
one-line fix as `session.rs` already uses.
For the record, I checked `sql`, `range`, `table`, and `empty_data_frame` in
`session.rs` and they are **lazy plan builders** with no network call, so
holding the GIL there is harmless — not a bug despite looking like one.
**Non-string map keys render as Rust Debug output.** `Value::Map` is
`BTreeMap<String, Value>`, and the new `MapArray` arm coerces non-string keys
with `format!("{other:?}")` (`dataframe.rs` ~2192). So a `map<int, string>`
column yields the key `"Integer(1)"` rather than `"1"`. Given this PR is
specifically about completing the Arrow mapping, that's worth either fixing
(format the scalar, not the enum) or noting as a known limitation.
**This branch is behind `master`.** `pr-54` forks from `9d01795` and is
missing three commits including the docs site (#53). No textual conflicts —
both sides touch `build_python_connect.yml` in different places — but it needs
a rebase, and after #53 the docs now describe the API this PR changes
(`functions` signatures in particular), so the docs pages may need a matching
update.
## What I did and didn't verify
I read the core diffs, compiled nothing against a live server, and did not
run the parity suite or the coverage script. The bug-fix claims above I
verified by reading `master` and the branch side by side; the gate behaviour I
verified by reading the scripts and the workflow invocation. I have not
independently confirmed that the 65 skiplisted tests do fail under the
reference client — that I'm taking from how the manifest was generated.
--
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]
