parthchandra commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5322206797
> Following up on test coverage, because I want to make sure I'm reading this right. > > As far as I can tell `SupportsReportOrdering` is what populates `BatchScanExec.ordering`, and that field is the only trigger for everything in this PR. Iceberg's implementation of it is [apache/iceberg#16750](https://github.com/apache/iceberg/pull/16750), which is still open and targeted at Spark 4.1, so on the published Iceberg versions in our poms (1.5.2, 1.8.1, 1.10.0, 1.11.0) `ordering` is always `None`. That means `reportableOrdering` returns `Nil`, `table_sort_orders` goes over empty, native takes `ordering = None`, and we get the old single-partition unordered scan with no `SortPreservingMergeExec`. > > If that's right, the effect in CI is bigger than the canceled plan assertions. The k-way merge itself never runs — every `checkSparkAnswer` in the new suite is exercising the read path we already had, so the only CI coverage of the new native code is the two Rust tests asserting `partition_count()`. > > There's a related wrinkle even on a reporting build: a global `ORDER BY` keeps its final sort, which would repair any mis-ordering the merge produced, so those tests catch dropped or duplicated rows but not wrong order. The order-sensitive assertions are all in the `assume`-gated SMJ/aggregate/window tests. > > Am I understanding the situation correctly, or is there something in the CI setup I'm missing that does exercise the merge? You're reading this right. I cannot think of a way to mock Iceberg without the actual implementation (which is still in review). The tests cover the fallback, and are somewhat forward looking. In a way this feature is itself forward looking - once this feature is released in Iceberg and Spark starts to eliminate sorts based on what iceberg-java reports, Comet will produce absolutely garbage results unless we have this implementation in place. -- 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]
