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

ENH: Clean up window translation logic in pyspark backend #2004

Merged

Conversation

icexelloss
Copy link
Contributor

@icexelloss icexelloss commented Oct 21, 2019

There are some code duplication in the current pyspark compiler. I cleaned up a bit.

@icexelloss
Copy link
Contributor Author

cc @hjoo @toryhaavik

@icexelloss icexelloss force-pushed the pyspark-backend-window-op-refactor branch 2 times, most recently from 9c61c8f to 259a801 Compare October 23, 2019 14:58
@icexelloss icexelloss force-pushed the pyspark-backend-window-op-refactor branch from 259a801 to 8e2790b Compare October 23, 2019 18:09
Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

@icexelloss thanks for working on that!
As we are using docstring with numpy format, could you provide these docstrings?

I hope in a near feature CI will do the docstring checks :)

ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
ibis/pyspark/compiler.py Show resolved Hide resolved
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

nice cleaning @icexelloss

@@ -1040,12 +1013,12 @@ def compile_lead(t, expr, scope, *, window, **kwargs):

@compiles(ops.MinRank)
def compile_rank(t, expr, scope, *, window, **kwargs):
return F.rank().over(window).astype('long') - F.lit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

for my own edification, what does F.lit(1) do in pyspark?

Copy link
Contributor Author

@icexelloss icexelloss Nov 7, 2019

Choose a reason for hiding this comment

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

F.lit(1) creates a constant literal column of "1"s

Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

LGTM. @icexelloss thanks for working on that.
related to my comments for docstring, no worry about that now.
I am working now to fix that.

@xmnlab xmnlab merged commit fb4652f into ibis-project:master Oct 31, 2019
@icexelloss
Copy link
Contributor Author

Thanks @xmnlab!

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

Successfully merging this pull request may close these issues.

3 participants