tdcmeehan commented on PR #103: URL: https://github.com/apache/spark-connect-rust/pull/103#issuecomment-6458087652
Thanks for this. Avoiding the IPC round-trip is a clear win, and the benchmarks make the case well. Before this lands, I'd like to question how much of the bridge needs to be custom. arrow-rs 58.3 already ships this handoff: `arrow::pyarrow::Table` implements `IntoPyArrow` via `pa.Table.from_batches`, on the same PyO3 0.28 line this workspace uses. As far as I can tell, the main reason to rebuild the descriptors here is the sorted-map flag that `TryFrom<&Field>` overwrites, which apache/arrow-rs#11630 fixes. But I don't think Spark can produce that case: - Spark always declares maps with `keysSorted = false` (`ArrowUtils.toArrowField`: `new ArrowType.Map(false)`), so there's no sorted flag to lose. - Connect results go through `ArrowConverters.toBatchWithSchemaIterator` → `ArrowUtils.toArrowSchema`, which never produces dictionary-encoded fields. So the dictionary-order checks in `schemas_equal` cover input the server doesn't send either. If that's right, `Table::try_new(batches, schema).map_err(arrow_error)?.into_pyarrow(py)` plus the NUL-name IPC fallback should give the same zero-copy result with much less code to maintain. It would also avoid a problem on the next upgrade: in arrow 60, `FFI_ArrowSchema.flags` is private, so the `schema.flags = source.flags` workaround won't compile. A couple of questions: 1. Was there a reason to avoid arrow-rs's export that I'm missing? The `_forbid_c_stream_import` fixture rules out the single-batch C stream that `RecordBatch::to_pyarrow` uses (a workaround for apache/arrow#37669). Is there a correctness or performance problem with that path? If so, it'd be worth a sentence in the PR description or the README. 2. Is there a Spark-produced schema (any server version) that hits the sorted-map or ordered-dictionary paths? If so, a test using that schema would document why the extra handling is needed. If not, I'd suggest dropping that handling and scoping the bridge to what Spark actually sends. -- 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]
