-
Notifications
You must be signed in to change notification settings - Fork 608
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
from __future__ import annotations | ||
|
||
import abc | ||
from typing import TYPE_CHECKING | ||
|
||
from ibis.common.grounds import Concrete | ||
|
||
if TYPE_CHECKING: | ||
from collections.abc import Sequence | ||
|
||
import ibis.expr.types as ir | ||
|
||
|
||
class Selector(Concrete): | ||
"""A column selector.""" | ||
|
||
@abc.abstractmethod | ||
def expand(self, table: ir.Table) -> Sequence[ir.Value]: | ||
"""Expand `table` into value expressions that match the selector. | ||
|
||
Parameters | ||
---------- | ||
table | ||
An ibis table expression | ||
|
||
Returns | ||
------- | ||
Sequence[Value] | ||
A sequence of value expressions that match the selector | ||
|
||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
from ibis.common.deferred import Deferred, Resolver, deferrable | ||
from ibis.common.exceptions import IbisInputError | ||
from ibis.common.grounds import Concrete | ||
from ibis.common.selectors import Selector # noqa: TCH001 | ||
from ibis.common.typing import VarTuple # noqa: TCH001 | ||
|
||
if TYPE_CHECKING: | ||
|
@@ -145,8 +146,8 @@ class WindowBuilder(Builder): | |
how: Literal["rows", "range"] = "rows" | ||
start: Optional[RangeWindowBoundary] = None | ||
end: Optional[RangeWindowBoundary] = None | ||
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]] = () | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 We're already calling |
||
|
||
@attribute | ||
def _table(self): | ||
|
There was a problem hiding this comment.
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 toir.Table
.I think
Selector
s could be viewed as another kind of deferred (or ratherResolver
) producing a sequence of outputs and deferreds are decoupled from their
.There was a problem hiding this comment.
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 asResolver
s could simplify the codebase, but that should be done in a separate PR.There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.