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

Accept rows-with-max-lookback as preceding parameter #1825

Merged
merged 3 commits into from
Jun 17, 2019

Conversation

emilyreff7
Copy link
Contributor

Support row-based trailing windows bound by a maximum look-back period

@cpcloud cpcloud self-assigned this Jun 12, 2019
@cpcloud cpcloud added feature Features or general enhancements window functions Issues or PRs related to window functions labels Jun 12, 2019
@cpcloud cpcloud added this to the Next Feature Release milestone Jun 12, 2019
ibis/expr/window.py Outdated Show resolved Hide resolved
ibis/expr/window.py Outdated Show resolved Hide resolved
ibis/expr/window.py Outdated Show resolved Hide resolved
ibis/expr/window.py Outdated Show resolved Hide resolved
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Last set of changes, then this is good to merge.

@@ -163,6 +203,12 @@ def _validate_frame(self):
"'how' must be 'rows' or 'range', got {}".format(self.how)
)

if self.max_lookback is not None:
if not isinstance(self.max_lookback, ir.IntervalValue):
Copy link
Member

Choose a reason for hiding this comment

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

I'll make a follow up for this, but I think we might also want to support passing in pandas.Timedelta objects here.

ibis.trailing_window(
rows_with_max_lookback(3, ibis.interval(days=3))
),
'rows between 3 preceding and current row',
Copy link
Member

Choose a reason for hiding this comment

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

I'll create a follow up issue for this as well, but I don't think we should implement this for any backends unless we can give it well-defined behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements window functions Issues or PRs related to window functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants