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

First version of omnisci window op #1771

Merged
merged 5 commits into from
May 16, 2019

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented May 10, 2019

This PR adds window operations for OmniSci/MapD backend:

  • lag
  • firstvalue
  • lead
  • lastvalue
  • mean
  • min
  • max
  • sum

@xmnlab xmnlab marked this pull request as ready for review May 11, 2019 01:51
@xmnlab xmnlab requested a review from cpcloud May 11, 2019 01:51
@cpcloud cpcloud added this to the Next Feature Release milestone May 11, 2019
@cpcloud cpcloud added mapd window functions Issues or PRs related to window functions labels May 11, 2019
@cpcloud
Copy link
Member

cpcloud commented May 11, 2019

@xmnlab Can you split this into 2 PRs? One with all the build/data loading related changes and one with the window changes?

@cpcloud
Copy link
Member

cpcloud commented May 12, 2019

@xmnlab Can you rebase here? Sorry for the merge conflicts, let me know if you'd like me to help resolve them.

@xmnlab
Copy link
Contributor Author

xmnlab commented May 13, 2019

@cpcloud ! sounds pretty good .. I will open a new PR with the changes related to new omnisci database docker. and after that I will move this PR forward :)

@xmnlab
Copy link
Contributor Author

xmnlab commented May 13, 2019

the PR for the omnisci ci: updating #1781

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #1771 into master will decrease coverage by 2.01%.
The diff coverage is 51.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
- Coverage   87.57%   85.56%   -2.02%     
==========================================
  Files          80       80              
  Lines       15145    15190      +45     
  Branches     1958     1969      +11     
==========================================
- Hits        13263    12997     -266     
- Misses       1529     1834     +305     
- Partials      353      359       +6
Impacted Files Coverage Δ
ibis/mapd/operations.py 71.32% <51.11%> (-2.51%) ⬇️
ibis/bigquery/client.py 41.1% <0%> (-53.39%) ⬇️
ibis/bigquery/compiler.py 59.92% <0%> (-37.5%) ⬇️
ibis/bigquery/udf/api.py 80.48% <0%> (-14.64%) ⬇️
ibis/bigquery/api.py 63.33% <0%> (-10.01%) ⬇️
ibis/impala/compiler.py 91.28% <0%> (-5.6%) ⬇️
ibis/pandas/client.py 85.63% <0%> (-3.45%) ⬇️
ibis/expr/schema.py 89.77% <0%> (-1.14%) ⬇️
ibis/expr/window.py 86.46% <0%> (-0.76%) ⬇️
ibis/expr/api.py 93.53% <0%> (-0.4%) ⬇️
... and 3 more

@xmnlab
Copy link
Contributor Author

xmnlab commented May 14, 2019

@cpcloud it seems it is done for a review :)

ibis/tests/all/test_window.py Outdated Show resolved Hide resolved
Move back some small changes

Fixed issue on omnisci config file

Fixed small issues; added protocol parameter

applying changes from upstream
@cpcloud
Copy link
Member

cpcloud commented May 15, 2019

Merging on green.

@cpcloud cpcloud self-assigned this May 15, 2019
@xmnlab
Copy link
Contributor Author

xmnlab commented May 15, 2019

thanks @cpcloud !

@cpcloud
Copy link
Member

cpcloud commented May 16, 2019

Just fixed up the merge conflict in the github UI, will wait for at least one the py36/py37 builds to turn green then will merge. Thanks @xmnlab!

@cpcloud
Copy link
Member

cpcloud commented May 16, 2019

@xmnlab Can you disable the any and all tests for mapd for now? I changed them to enable testing without calling into the cumulative operations interface to be more consistent with the rest of the test cases in that file.

Sorry for the churn!

@xmnlab
Copy link
Contributor Author

xmnlab commented May 16, 2019

@cpcloud sure .. I will do it now. thanks!

@xmnlab
Copy link
Contributor Author

xmnlab commented May 16, 2019

not sure why it broke just for py35 with this error:

INTERNALERROR> AssertionError: ('ibis/tests/all/test_window.py::test_window[MapD-cumany]', <WorkerController gw1>)
INTERNALERROR> assert not 'ibis/tests/all/test_window.py::test_window[MapD-cumany]'

maybe I should just add these ops as unsupported for window operations

@xmnlab
Copy link
Contributor Author

xmnlab commented May 16, 2019

@cpcloud it is done :)

@cpcloud
Copy link
Member

cpcloud commented May 16, 2019

Thanks, I will merge when I get to the office around 10a Eastern!

@xmnlab
Copy link
Contributor Author

xmnlab commented May 16, 2019 via email

@cpcloud cpcloud merged commit c2323b8 into ibis-project:master May 16, 2019
@xmnlab xmnlab deleted the add-mapd-window-support branch May 16, 2019 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
window functions Issues or PRs related to window functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants