zhengruifeng commented on PR #58159:
URL: https://github.com/apache/spark/pull/58159#issuecomment-5364758579
I took a quick look at the performance tradeoff here. The memory direction
looks good, but I think this needs benchmark coverage before merge because the
current implementation may trade a sizeable amount of CPU for the lower peak
Python memory.
The key shape change is:
```python
# old
rows = [to_row(item) for item in data]
pylist = [
[conv(row[i]) for row in rows] if conv is not None else [row[i] for row
in rows]
for i, conv in enumerate(column_convs)
]
# new
pylist = [[] for _ in range(len_column_names)]
for item in data:
for i, value in enumerate(to_row(item)):
conv = column_convs[i]
pylist[i].append(conv(value) if conv is not None else value)
```
I ran a small local micro-benchmark of just this Python
conversion/transposition shape, including `pa.Table.from_arrays`, with 300k
rows and schema `(LongType, StringType, DoubleType)`. This was not Spark ASV
and not a Connect end-to-end benchmark, so treat it as a signal only:
```text
tuple input: old 0.081s, 9.9 MiB peak -> new 0.349s, 7.4 MiB peak
Row input: old 0.344s, 28.2 MiB peak -> new 0.504s, 7.4 MiB peak
dict input: old 0.533s, 28.2 MiB peak -> new 0.673s, 7.4 MiB peak
```
So the PR does reduce peak Python allocations substantially for
Row/dict-style inputs, where the old `rows` list retained normalized tuples for
the whole batch. For plain tuple input the memory saving is smaller because
`tuple(existing_tuple)` reuses the tuple object and the old `rows` list mostly
stores references.
The concern is wall-clock time: the new nested-loop shape replaces
column-wise list comprehensions with Python-level `append` and per-cell
converter checks. That is also notable because SPARK-52796 previously optimized
this area by moving toward list comprehensions and away from repeated append
calls.
Could you add an ASV benchmark, ideally covering both
`LocalDataToArrowConversion.convert` directly and Spark Connect
`createDataFrame` end-to-end, across tuple / Row / dict inputs and varying row
counts? That would make the memory-vs-CPU tradeoff explicit.
--
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]
