yangshangqing95 commented on code in PR #18011: URL: https://github.com/apache/iceberg/pull/18011#discussion_r3971979335
########## spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/DeleteOrphanFilesSparkAction.java: ########## Review Comment: Could we also account for the stream_results=true path here? For non-streaming execution this looks good: collectAsList() still evaluates the full Dataset once before deletion, so skipping the eager count() removes an extra action/cache without changing the ordering of discovery vs deletion. Streaming is different, though. deleteFiles consumes this Dataset through toLocalIterator(). Spark documents that toLocalIterator() triggers multiple jobs and specifically recommends caching when the input is the result of a wide transformation such as a join to avoid recomputation. Here the returned Dataset is directly based on joinWith, so returning it uncached for IGNORE / DELETE may trade the cache pressure for repeated upstream work. There is also a behavioral change in the streaming path: today the eager count() completes the entire orphan-file computation before any deletion starts. With a lazy toLocalIterator(), deletion can begin after the first batches are produced while later partitions are still being evaluated. A later listing/join/task failure could therefore leave the action partially applied, whereas previously that evaluation failure occurred before deletion. Could we add coverage for the streaming path and clarify whether these two trade-offs are intentional? I don't think the existing collectAsList() laziness test exercises this interaction. -- 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]
