viirya opened a new pull request, #89:
URL: https://github.com/apache/spark-connect-rust/pull/89
### What changes were proposed in this pull request?
Add an explicit `module = "..."` to every `#[pyclass]` in
`crates/pyspark-rs/src` (99
of them, including the `abstract_type!` macro that generates the 8
intermediate
DataType bases), and set the module on `create_exception!` for
`SkipRestOfInputTableException`.
PyO3 defaults a `#[pyclass]`'s `__module__` to `builtins` when no `module`
argument is
given, and none of ours set it.
The class-to-module mapping is derived from where our own drop-in skin
(`python/pyspark/**`) already re-exports each class, cross-checked against
Apache Spark
v4.2.0 (commit `32f72996011` — the same commit pinned in
`crates/spark-connect-proto/proto/PROTO_SHA.txt`).
Two mapping notes:
- `DataFrame` and `Column` map to `pyspark.sql.connect.*`, **not**
`pyspark.sql.*`.
Upstream's `pyspark/sql/dataframe.py:78` and `pyspark/sql/column.py:41`
hold the
*abstract* parent; the concrete class a Connect session hands the user is
`pyspark/sql/connect/dataframe.py:112` / `.../connect/column.py:109`,
which is what
`type(df).__module__` reports on a real Connect session.
- `RustRpcError` deliberately keeps `_pyspark`: it is an internal transport
error with
no upstream counterpart, so asserting a pyspark path for it would be false.
The hand-written `__reduce__` impls in `types.rs` are **kept**. They carry
the type's
parameters (`DecimalType(12,3)`, `ArrayType(...)`), which a by-reference
pickle of the
bare class would not restore.
### Why are the changes needed?
`__module__ == "builtins"` is a behavioral divergence from the reference
client, not
just a cosmetic one:
1. **The class itself was unpicklable.** pickle serializes a class *by
reference* —
writing `__module__` + `__qualname__` and re-importing on load — so the
lookup
landed in `builtins` and failed:
```
>>> import pickle
>>> from pyspark.sql.types import IntegerType
>>> pickle.dumps(IntegerType)
PicklingError: Can't pickle <class 'builtins.IntegerType'>:
attribute lookup IntegerType on builtins failed
```
Real pyspark pickles these fine. Reproduced for `DataFrame`, `Column`,
and every
`DataType` class.
2. **Error messages, reprs, and `__module__`-based dispatch showed
`builtins.DataFrame`**
instead of the pyspark path — user-visible, and it undercuts the drop-in
premise.
Instance pickling already worked, but only because each DataType hand-writes
a
`__reduce__` routing through
`pyspark.sql.types._parse_datatype_json_string`. The cost
of the wrong `__module__` was therefore already being paid, worked around
per class
rather than fixed at the root.
This is the same "declared but never applied" gap as SPARK-59032 (parsed
gRPC keepalive
never applied to the channel) and SPARK-59037 (128 MiB max message size
declared but
unused): the intent existed, but the wiring that makes it take effect was
missing.
### Does this PR introduce _any_ user-facing change?
Yes, and it is the point of the change: the drop-in's classes now report the
same
`__module__` as the reference client (`pyspark.sql.types.IntegerType` rather
than
`builtins.IntegerType`), so pickling a class works and error messages/reprs
show the
pyspark path. No API surface is added or removed.
### How was this patch tested?
Four new offline tests in `python/tests/test_dropin_offline.py`:
- the module paths for `DataFrame` / `Column` / `SparkSession` / `DataType` /
`IntegerType` / `StructType` / `NumericType` (macro-generated base) /
`Row`,
- that the DataType classes pickle by reference and round-trip to the
*identical*
class object,
- and, as a guard against a future `#[pyclass]` landing without `module`,
that no class
exported by `pyspark._pyspark` reports `builtins`.
Verified manually against a locally built extension: all **108** exported
classes are
covered, none left on `builtins`. Regression-checked that the existing
`__reduce__`
instance path is unaffected — `DecimalType(12,3)` → `decimal(12,3)`, nested
`StructType`, the UDF cloudpickle payload, `isinstance`, and the MRO all
behave as
before.
`cargo test` passes (93 core tests + all suites, 0 failures). `cargo clippy
-p
pyspark-rs` reports **389 warnings both before and after** this change — i.e.
unchanged from master, nothing introduced (all pre-existing
`Default::default()` /
large-`Err` lints). `cargo fmt --check` is clean.
Not run in my local environment (no `pytest`/`pandas` on the system Python):
the full
`test_dropin_offline.py` suite and the parity gates — those need CI. I
verified the new
tests' assertions by executing their logic directly against the built
extension.
--
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]
