NGA-TRAN commented on issue #24438: URL: https://github.com/apache/datafusion/issues/24438#issuecomment-5531094315
Thanks @alamb — the GROUPED model is a nice way to formalize what we’ve been calling group-contiguous. Two quick clarifications, then a question: `SortProperties` already lives in DataFusion (datafusion-expr-common), not arrow-rs. The arrow type is `SortOptions` (`ASC`/`DESC` + nulls). `GROUPED` is not a total order, so we would not put it on `PhysicalSortExpr` / `SortOptions` (that would touch EnforceSorting, SMJ, proto, etc.). If we go the `GROUPED` route, the implementation wI’d propose is: 1. Keep real sorts as they are (`PhysicalSortExpr` + `oeq_class`). 2. Add a sibling `geq_class` on `EquivalenceProperties`: a lex tuple of exprs that are contiguous but not ordered. Every existing ordering implies a grouping (drop ASC/DESC). 3. Add `SortProperties::Grouped` only for expression propagation (`date_bin` already copies its input’s sort property). 4. Sources (esp. `FileScanConfig`, when concat drops `output_ordering` but ranges are disjoint) advertise the weaker grouping. 5. `AggregateExec` uses that for group completion. We would still want Xavier’s #24697 split (`GroupCompletionMode` ≠ `InputOrderMode`). #24698’s sidecar on `PlanProperties` would not be needed. #24698 is narrower and less work; `GROUPED` on `EquivalenceProperties` is the more general property. Which would you like us to pursue — implement `GROUPED` as above, or continue with Xavier’s #24698 / #24497 stack? -- 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]
