moomindani commented on PR #16933: URL: https://github.com/apache/iceberg/pull/16933#issuecomment-4949229291
Thanks for working on this — the driver-side serial listing in the `usePrefixListing` path is a real bottleneck and the executor fan-out direction makes sense. A few concerns, mostly about scope and defaults: 1. **This PR bundles three independent changes**, and two of them aren't mentioned in the description: (a) executor-side bulk deletion, which now kicks in by default for every bulk-IO table regardless of listing mode, and (b) `cache()` -> `persist(DISK_ONLY)` in `findOrphanFiles`. Could you split those into separate PRs? They each deserve their own discussion, and reviewing the listing change alone would be much easier. 2. **The executor-side deletion changes the result semantics.** Today, non-streaming mode returns all orphan file locations in `Result.orphanFileLocations`; the new path always returns at most `max-orphan-file-sample-size` (20k) paths and silently ignores `stream-results`. That's a user-visible behavior change enabled by default. 3. **The sub-prefix discovery reintroduces the Hadoop `FileSystem` dependency on the driver.** Prefix listing was added in #12254 specifically so this action works with FileIO-only setups (e.g. REST catalog + `S3FileIO` with `s3://` paths and no `fs.s3.impl` configured). With `parallel-prefix-listing` defaulting to `true`, those existing `prefix_listing => true` users would start failing at the discovery step. I'd suggest either defaulting to `false` until discovery works without Hadoop, or discussing an extension to `SupportsPrefixOperations` for single-level (delimiter-based) listing — S3 `ListObjectsV2` supports this natively. 4. Minor: the new progress logs use `LOG.warn` for routine progress (existing code uses `LOG.info`), and the `parallel-prefix-listing=false` fallback path has no test coverage. Also, new functionality usually lands in the latest Spark version first (v4.1) and is then backported — worth flipping the order once the approach settles. -- 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]
