andygrove commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5299335004
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, 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? -- 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]
