andygrove commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5684846260
I built Iceberg from apache/iceberg#16750 plus apache/iceberg#14948 and ran this suite against it with a probe at the native merge-or-sort decision. Every test on an unpartitioned table runs the old unordered read, because `isOrderingEnabled` in the Iceberg PR needs a non-empty grouping key type and Iceberg's own `testNoMergeReaderForUnpartitionedSortedTable` asserts exactly that. That covers "merges multiple sorted files", the duplicate-key, MOR-delete, descending, multi-column, single-file, on-versus-off and order-by-limit tests, and both 70-file tests. The above-the-cap sort fallback therefore has no end-to-end coverage at all. I also ran two mutations. Making every ordered partition read every file was caught by the NULLS FIRST, NULLS LAST, partitioned-table, SMJ and group-by tests only. Flipping the merge direction so rows come out complete but mis-ordered was caught by the SMJ test alone, since every test with a global ORDER BY has its order repaired by Spark's final sort. Could the merge tests move to the shape the NULLS tests already use, a table partitioned by a constant column with preserve-data-grouping on, and add an `assumeOrderingReported` after the correctness check so a reporting build proves the merge ran? For the 70-file tests the same change would make the cap boundary real. And for order sensitivity, a join, a window, or a `SortAggregate` over the merged input is what actually verifies the order, since ORDER BY cannot. One thing to be aware of on the Iceberg side: with those two PRs as they stand, Spark's own answer is nondeterministically wrong. The window test failed for me with Spark returning `row_number` 2 for the first row, and a run with Comet's sort path untouched had Spark return 4 join rows where 6 is correct. Which revisions did you validate with? -- 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]
