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: [Omnisci] fillna and nullif implementations #2083

Closed
wants to merge 6 commits into from

Conversation

semelianova
Copy link
Contributor

Omnisci doesn't support ISNULL() function, but it is possible to replace it with another sql request.

@semelianova
Copy link
Contributor Author

semelianova commented Feb 17, 2020

CI doesn't pass with 5.0.0 omnisci release, but test passes with the newest master. Non-empty LogicalValues are supported since commit, which isn't included in the latest release v5.1.0

@pep8speaks
Copy link

pep8speaks commented Feb 20, 2020

Hello @semelianova! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 31:21: F821 undefined name 'OmniSciDB'

Comment last updated at 2020-03-26 13:09:36 UTC

@anmyachev
Copy link
Contributor

@semelianova use omnisci/core-os-cpu-dev:latest and fix pep8 errors

@anmyachev
Copy link
Contributor

@semelianova should ibis/tests/all/test_generic.py::test_fillna_nullif pass since your change?

@semelianova
Copy link
Contributor Author

@anmyachev Not completely, only 2 out of 4 cases which test fillna

@semelianova semelianova changed the title FEAT: [Omnisci] Fillna implementation FEAT: [Omnisci] fillna and nullif implementations Feb 26, 2020
@semelianova
Copy link
Contributor Author

@xmnlab @jreback PR is ready for review

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.

@semelianova LGTM. thanks for working on that.

just a comment, it is not a block for merging, but maybe if you change from _ifnull_workaround to _ifnull it would look better ... there are more operations that do a "workaround" but you don't need to add this to the function name.

could you add a release note for your changes please?

@xmnlab
Copy link
Contributor

xmnlab commented Feb 27, 2020

@jreback when you have time could you give a second review pls? thanks!

@xmnlab
Copy link
Contributor

xmnlab commented Mar 2, 2020

@semelianova I will ping here when #2084 is merged.

@xmnlab xmnlab added feature Features or general enhancements omnisci labels Mar 6, 2020
@jreback
Copy link
Contributor

jreback commented Mar 12, 2020

can you merge master, remove the updated image, and update the release note

@semelianova
Copy link
Contributor Author

Need to wait for #2084 is merged

@anmyachev
Copy link
Contributor

Need to wait for #2084 is merged

#2084 was be merged

@jreback
Copy link
Contributor

jreback commented Mar 23, 2020

can u merge master

checks are failing

@dchigarev dchigarev mentioned this pull request Mar 27, 2020
@xmnlab
Copy link
Contributor

xmnlab commented Mar 27, 2020

@semelianova please try to use the omniscidb image tag used by master d7e34d4
the tag you set doesn't exist

@anmyachev
Copy link
Contributor

@xmnlab this PR can be closed :)

@xmnlab
Copy link
Contributor

xmnlab commented Apr 3, 2020

thanks @semelianova @anmyachev !

@xmnlab xmnlab closed this Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants