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

[FEA] no seriously we should try to just stop using bitwiseMergeAndSetValidity #7698

Open
revans2 opened this issue Feb 7, 2023 · 2 comments
Labels
feature request New feature or request

Comments

@revans2
Copy link
Collaborator

revans2 commented Feb 7, 2023

Is your feature request related to a problem? Please describe.
I filed #7485 to fix some potentially problematic cases with biwiseMergeAndSetValidity. Originally I wanted to stop using the API wherever possible, but the fix for the operator itself is not that bad, so we decided to go that route. But this is a really dangerous API and I just don't know if the performance savings using it outweigh the inherent danger in it.

Like if the column that is passed in, which will have validity set for it, has nulls in it at all, this API can stomp over the top of it and make it not null. This can result in us using/reading bogus data.

I still think we should try to stop using this API and run some performance tests to see what the impact is of moving away from it.

@razajafri
Copy link
Collaborator

This is fixed by rapidsai/cudf#13335.
@revans2 can you please confirm?

@revans2
Copy link
Collaborator Author

revans2 commented May 16, 2023

@razajafri #7485 is fixed by rapidsai/cudf#13335.

It also helped to make the API safer and showed that there is a performance improvement over doing an if/else (20% to 40% better). So it makes me rethink this a bit, but I still don't like bitwiseMergeAndSetValidity as an API.

It it is confusing to use, especially compared to something like if/else. So as an implementation I think rapidsai/cudf#13335 has addressed my concerns. And an API no it has not.

@revans2 revans2 reopened this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants