gimgit commented on PR #16305: URL: https://github.com/apache/iceberg/pull/16305#issuecomment-5354594611
I've been working in the same area (#17504, overlap measurement/selection on the sort key), so I read this with interest. K-way merge for pre-sorted files fills a real gap — sort compaction re-shuffles data that is already 99% ordered, and that cost is what pushes people to skip maintenance. A few questions and one coordination note: 1. **File-level sortedness.** The streaming merge assumes each input file is internally sorted, but table sort order is advisory — an engine or a plain append can write an unsorted file into a sorted table, and nothing in the metadata records per-file sortedness. If one input file is unsorted, the merge output is silently mis-ordered. Is there a validation pass, or is this documented as a precondition? A cheap partial guard could be checking that each file's bounds don't contradict its neighbors, though that can't see inside a file. 2. **Selection.** The planner extends size-based selection, but the population this strategy targets — pre-sorted, size-healthy files that overlap on the sort key — is exactly the set that size-based selection can't see (reproduction in #17489). With size thresholds alone, the files that most need a k-way merge may never be picked. The `min-overlap-depth` option proposed in #17504 measures that state from the same bounds you already load; the two would compose naturally (detect by overlap, rewrite by k-way merge). Happy to coordinate if that's useful. 3. **Lower-bound-only ordering.** Sorting by lower bound before bin-packing keeps adjacent-key files together, but nested ranges break it: a file spanning [0, 1000] sorts next to [1, 2], [3, 4]..., and the wide file drags overlap into whichever group it lands in. Considering the upper bound too (e.g., sorting by midpoint, or splitting wide files into their own group) might reduce cross-group overlap. We hit the same co-grouping problem on the selection side, so no clean answer here either — flagging it as a shared open question. 4. **Coordination note.** This PR adds a `buildTableScan()` hook on `BinPackRewriteFilePlanner` to call `includeColumnStats()`; #17504 adds a `columnsToIncludeStats()` hook on the same class for the same reason. One of us will conflict with the other. Also, the no-arg `includeColumnStats()` retains bounds for every column of every task through planning — on a wide schema that gets heavy. `includeColumnStats(Collection)` scoped to the sort column avoids it; that's what #17504 ended up doing after hitting the memory question. -- 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]
