Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): support selectors in window function order_by and group_by #9649

Merged

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jul 21, 2024

Support selectors in window function group_by and order_by arguments. Closes #9637.

@cpcloud cpcloud added this to the 9.2 milestone Jul 21, 2024
@cpcloud cpcloud added feature Features or general enhancements ux User experience related issues labels Jul 21, 2024
@cpcloud cpcloud force-pushed the group-by-order-by-window-selectors branch from 0f0cca6 to 2d4a7ba Compare July 21, 2024 14:49
if TYPE_CHECKING:
from collections.abc import Sequence

import ibis.expr.types as ir
Copy link
Member

@kszucs kszucs Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to move the base selector class to ibis.common since it is tied to ir.Table.
I think Selectors could be viewed as another kind of deferred (or rather Resolver) producing a sequence of outputs and deferreds are decoupled from the ir.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly even bind() would properly handle that, so refactoring selectors as Resolvers could simplify the codebase, but that should be done in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the class to avoid an import cycle and having it anywhere else didn't really fit.

If you have another suggestion that isn't to refactor the whole class :) I'm happy to hear it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, I wasn't aware of the import cycle.

ibis/expr/builders.py Outdated Show resolved Hide resolved
@cpcloud cpcloud force-pushed the group-by-order-by-window-selectors branch from 2d4a7ba to 85faac8 Compare July 22, 2024 11:34
@cpcloud cpcloud requested a review from gforsyth July 22, 2024 13:33
groupings: VarTuple[Union[str, Resolver, ops.Value]] = ()
orderings: VarTuple[Union[str, Resolver, ops.SortKey]] = ()
groupings: VarTuple[Union[str, Resolver, Selector, ops.Value]] = ()
orderings: VarTuple[Union[str, Resolver, Selector, ops.SortKey]] = ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's a bit of noise from dealing with the import cycle from using Selector at the top level of builders.py, I'm calling out that this is the change that enables use of a Selector in the group_by and order_by window APIs.

We're already calling Table.bind on these two fields in the appropriate place, we simply needed to allow them in the signature of this builder to allow them to get passed through the API.

@cpcloud cpcloud force-pushed the group-by-order-by-window-selectors branch from 85faac8 to 60fa0b1 Compare July 22, 2024 14:57
@cpcloud cpcloud merged commit 0ad47de into ibis-project:main Jul 22, 2024
82 checks passed
@cpcloud cpcloud deleted the group-by-order-by-window-selectors branch July 22, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(api): support selectors in window function group_by and order_by arguments
3 participants