Changes between Initial Version and Version 1 of Ticket #35339, comment 2
- Timestamp:
- Mar 30, 2024, 7:34:52 PM (6 months ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
Ticket #35339, comment 2
initial v1 3 3 Since this a long standing issue and we thus don't have to deal with a backport I would suggest we bite the bullet and refactor things up to have `OrderableAggMixin` merged in `Aggregate` and have a flag similar to `allow_distinct`. 4 4 5 The crux of the issue here is that we try be clever and have the `get_source_expressions` / `set_source_expressions` signature change depending on whether or not a `filter` and and `order_by` are assigned (the `len` of expressions will change) so we can satisfy `list[Expression]` . We tried doing that for a while with `Window` but it became clear that using `None` as placeholders for unspecified expressions was much easier to reason about pretty much everything expects `get_expressions` to be `list[Expression | None]`. Doing so avoids the conditional shenanigans that are causing out of order due to `expressions.pop` and such.5 The crux of the issue here is that we try be clever and have the `get_source_expressions` / `set_source_expressions` signature change depending on whether or not a `filter` and and `order_by` are assigned (the `len` of expressions will change) so we can satisfy `list[Expression]` but with a variadic length. We tried doing that for a while with `Window` but it became clear that using `None` as placeholders for unspecified expressions was much easier to reason about pretty much everything expects `get_expressions` to be `list[Expression | None]`. Doing so avoids the conditional shenanigans that are causing out of order due to `expressions.pop` and such. 6 6 7 7 Another big plus to having all the logic lives in `Aggregate` is that it makes implementing things like `STRING_AGG` (AKA `GROUP_CONCAT`) and [https://sqlite.org/releaselog/3_44_0.html other expressions that gain support for ordering overnight much easier] and tested in a generic way.