parthchandra commented on PR #5331: URL: https://github.com/apache/datafusion-comet/pull/5331#issuecomment-5942037116
> Summary > > * Prior state and problem: Native Iceberg scans discarded file ordering, preventing safe use of sorted input by downstream operators. > * Design approach: Carry Iceberg’s reported ordering through Scala and protobuf, then merge sorted file streams within each Spark partition. > * Correctness / compatibility analysis: No additional introduced P1/P2 issues found within this review. The existing [P1 composite floating-point ordering issue](https://github.com/apache/datafusion-comet/pull/5331#discussion_r4104363485) remains reproducible. Iceberg can order `(-0.0, 2)` before `(+0.0, 1)`, while Spark treats the zeros as equal and requires the opposite order on the second key. The merge does not repair this, potentially producing incorrect top-K results. Checked relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and current 4.2 development, plus Iceberg’s comparators and ordering reporter. > * Key design decisions: Planning-time binding aligns advertised and serialized ordering. Reusing `SortPreservingMergeExec` and `SortExec` keeps the implementation straightforward without introducing a custom merge abstraction. > * Implementation sketch: Each file becomes a native input partition beneath the merge. Above `maxFilesPerPartition`, the planner uses an unordered scan followed by a spillable sort. > * Behavioral changes worth calling out: Disabling merging still preserves ordering through sorting. The existing [unprojected-key fallback](https://github.com/apache/datafusion-comet/pull/5331#discussion_r4073739807) and [repeated shared-delete loading](https://github.com/apache/datafusion-comet/pull/5331#discussion_r3962418695) concerns remain substantiated and unresolved. The latter rebuilds the delete cache per file, retaining the documented I/O regression. > * Suggested improvements: Resolve the existing threads by sorting unsafe composite floating-point orderings, preserving native scans where unprojected ordering cannot satisfy a consumer, and sharing delete-loading state across file streams. > > Reviewed all 12 changed files across the full diff from `4c2ab9686526f30bc782462353e3dc97e3954b3f` to `53d50136507a44e5d8c941f55b1852c8c4d34bab`. The PR is not a draft. Read existing discussions and applied `.ai/skills/review-comet-pr/SKILL.md`. No sibling skill applies. > > Exact-head CI: 55 successful checks, 9 skips, no failures. Rust CI reports 1,458 passed tests and 5 skips. The new Scala suite passed 17 tests and canceled 15 because published Iceberg does not report ordering. > > Validation: Reused recorded exact-head evidence for 13 passing focused native tests with `--no-default-features` and the real-Parquet reproduction, verifying the saved planner matches this checkout. Reran the saved bounded merge probe and confirmed the signed-zero ordering mismatch. No fresh full native/JVM build or integration run with an ordering-reporting Iceberg build was performed in this pass. The checkout remains clean. No GitHub state was changed. Thanks @sunchao . Status on the three open items: the floating-point composite sort key is fixed (see the planner.rs thread — it now takes the full sort path). The unprojected sort key no longer forces a Spark fallback — the scan stays native and just doesn't report an order (see the CometScanRule thread). The repeated delete-file loading now has its own issue, #6524, so it isn't lost. -- 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]
