viirya commented on PR #103:
URL:
https://github.com/apache/spark-connect-rust/pull/103#issuecomment-6461931458
Following up on the question about `arrow::pyarrow::Table`: I think
switching would shrink this PR to roughly the `toArrow` change plus the NUL
fallback.
The core of `toArrow` would become:
```rust
let batches = py.detach(||
self.dataframe.collect_record_batches()).to_pyerr()?;
let schema = batches.first().map(|b| b.schema()).unwrap_or_else(||
Arc::new(Schema::empty()));
match Table::try_new(batches.clone(), schema.clone()) { // Arc clones, kept
for the fallback
Ok(table) => match table.into_pyarrow(py) { ... } // NUL
name/timezone -> IPC fallback
Err(e) => Err(PyValueError::new_err(e.to_string())),
}
```
That is roughly 20 lines in `pyspark-rs/src/dataframe.rs`, with `arrow`'s
`pyarrow` feature enabled. At that size I don't think a separate crate is worth
it. Without one, the following could go:
- `crates/spark-python-arrow/` entirely: `bridge.rs`, `lib.rs`,
`Cargo.toml`, `README.md`, and both test files.
- `.github/workflows/python-arrow.yml`. It exists only because the crate's
embedded-Python tests can't be built together with `pyspark-rs`
(extension-module feature unification).
- The workspace member entry, the `pyspark-rs` path dependency, and most of
the architecture doc changes.
Most of the tests would go with the crate. In `bridge_regressions.rs`, two
tests would fail against the upstream export rather than just becoming
redundant:
- `table_export_preserves_sorted_map_flags_in_nested_fields` expects
`keys_sorted=True`. arrow 58's `TryFrom<&Field>` still overwrites that flag
(fixed by apache/arrow-rs#11630).
- `table_export_rejects_inconsistent_nested_dictionary_order` expects a
`ValueError`. `Table::try_new` compares schemas with `Schema ==`, and
arrow-rs's `Field` equality ignores `dict_is_ordered`.
Spark never produces either input: maps are always written with `keysSorted
= false`, and Connect results contain no dictionary-encoded fields. So both
tests can simply be deleted. The other two test arrow-rs's own FFI behavior:
ordered-dictionary flags, and sliced validity bitmaps, which arrays decoded
from IPC don't have anyway. Those belong upstream.
What I think is worth keeping is the NUL fallback tests and one
buffer-address assertion, so a future change can't quietly reintroduce the IPC
copy. Both fit in `python/tests/test_arrow_handoff.py`.
--
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]
