HyukjinKwon commented on PR #59551: URL: https://github.com/apache/spark/pull/59551#issuecomment-6225882615
@viirya Thanks for following up. I agree that the copy changes what path 1 is. The difficulty is that keeping the pass-through columns as they are is only correct if a batch stays valid until its result comes back, which `ColumnarBatch` does not promise, and which no source can promise today: since SPARK-55715, `DataSourceRDD` closes a reader as soon as it is exhausted, so even the source of the benchmark, which allocates new vectors for every batch, frees them before the last results come back. So I see these options, with the numbers above: 1. Copy the pass-through columns, as this PR does: 1458 ms, 1.14x faster than without columnar input, but with an allocator outside memory accounting, the list views, and the lifecycle code. 2. Send these batches to path 2: correct and accounted for, but 1862 ms, slower than without columnar input. 3. Keep path 1 as it is, only for sources that declare that their batches stay valid until the end of the task, which would be a new opt-in, and otherwise not take columnar input: 1661 ms for the others. 4. Disable `spark.sql.execution.arrow.pythonUDF.columnarInput.enabled` by default as the fix for 4.2, and settle the design of path 1 separately. Which would you prefer? For the backports, 4 is the smallest change, and 3 needs a new API. I'm happy to switch this PR to either. -- 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]
