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: Support custom window in pandas backend #2260

Merged
merged 5 commits into from
Jul 1, 2020

Conversation

icexelloss
Copy link
Contributor

@icexelloss icexelloss commented Jun 30, 2020

This PR refactors the existing pandas window execution to allow implementing custom window with pandas backend.

The change is tested with a new test test_custom_window_udf which implements a dummy custom window and test it with pandas backend.

@icexelloss icexelloss added the pandas The pandas backend label Jun 30, 2020
Copy link
Contributor

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, added few minor suggestions.

I guess we want to add a release note for this, can you add it please? Thanks!

ibis/pandas/execution/window.py Outdated Show resolved Hide resolved
ibis/pandas/execution/tests/test_window.py Outdated Show resolved Hide resolved
ibis/pandas/execution/tests/test_window.py Outdated Show resolved Hide resolved
ibis/pandas/execution/tests/test_window.py Outdated Show resolved Hide resolved
expected_1 = pd.Series([4.0, 10.0, 5.0])

tm.assert_series_equal(result_0, expected_0)
tm.assert_series_equal(result_1, expected_1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it'd be more conventional to use a fixture for all the custom objects, and parameterize this. But happy to have it like this, if we don't expect to reuse all the custom objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

can also parametrize this test with the different cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made those fixtures

@icexelloss
Copy link
Contributor Author

Thanks @jreback @datapythonista for the review! I addressed all the comments. Please take another look when you got the chance, thank you!

Copy link
Contributor

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @icexelloss

@jreback jreback added this to the Next Bugfix Release milestone Jul 1, 2020
@jreback jreback merged commit f84384c into ibis-project:master Jul 1, 2020
@jreback
Copy link
Contributor

jreback commented Jul 1, 2020

thanks @icexelloss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pandas The pandas backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants