namanjain24-sudo commented on issue #16297: URL: https://github.com/apache/datafusion/issues/16297#issuecomment-5635379391
I looked into whether any part of this could land ahead of the Substrait change @kosiew linked. Short answer: no, and I think it is worth writing down why so the next person does not redo it. **Still blocked, as of today.** `BoundsType` on substrait main is still only `UNSPECIFIED`, `ROWS`, `RANGE`, and the window function spec is now explicit that there is no third option: > Producers must set `BoundsType` to `BOUNDS_TYPE_ROWS` or `BOUNDS_TYPE_RANGE`. So `to_substrait_bound_type` returning `not_impl_err!` is the correct behaviour right now, not a gap to paper over. substrait-io/substrait#902 is still open with no discussion. **There is a partial rewrite, and it does not help.** A `GROUPS` frame whose bounds are only `UNBOUNDED PRECEDING`, `CURRENT ROW` or `UNBOUNDED FOLLOWING` is the same window as the identical `RANGE` frame. That falls straight out of the same spec paragraph: > `CurrentRow` identifies the current record for ROWS bounds and the current peer group, whose records have equal sort key values, for RANGE bounds I checked it against DataFusion rather than trusting the reading, on `aggregate_test_100` ordered by `c2`, which has five values with duplicates so peer groups are real. Comparing `GROUPS` against `RANGE` per row: | frame | rows where GROUPS and RANGE differ | | --- | --- | | `UNBOUNDED PRECEDING AND CURRENT ROW` | 0 | | `CURRENT ROW AND UNBOUNDED FOLLOWING` | 0 | | `UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING` | 0 | | `GROUPS CURRENT ROW` vs `RANGE CURRENT ROW` | 0 | The same comparison against `ROWS` differs on 95 of 100 rows, so the zeros are not vacuous. This rewrite is also safe with a multi-column `ORDER BY`: the single-ordering-expression restriction on `RANGE` applies only when a bound is `Preceding` or `Following`. The reason it is not worth doing: of the 14 queries in `test_files/*.slt` that use `GROUPS`, 13 contain at least one offset bound, and the one that does not is a `statement error` case. So the rewrite would not turn a single one of the failing cases green. It only pays off alongside the spec change, or not at all. **One note for whoever picks this up.** substrait-io/substrait#902 describes DataFusion as already interpreting `GROUPS` on the consumer side "by mapping the raw integer to an internal extended enum". I could not find that on main. `from_window_function` goes through `BoundsType::try_from(window.bounds_type)` and fails with `Invalid bound type` for anything outside 0/1/2, and the only mention of `GROUPS` in `datafusion/substrait/` is the `// TODO` next to the error in the producer. Worth not counting on that workaround existing. -- 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]
