-
Notifications
You must be signed in to change notification settings - Fork 908
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
Ensure we pass the has_nulls tparam to mixed_join kernels #16708
Ensure we pass the has_nulls tparam to mixed_join kernels #16708
Conversation
/ok to test |
Do we not have a simple gtest for this? |
I was able to build our plugin with a cuDF that has this change and it fixes our issue. |
There is code in mixed_join_tests that should test the null path: https://github.com/rapidsai/cudf/blob/branch-24.10/cpp/tests/join/mixed_join_tests.cu#L711, and the not null path: https://github.com/rapidsai/cudf/blob/branch-24.10/cpp/tests/join/mixed_join_tests.cu#L680. I think..., I am not familiar with this code, so I could be missing it. That said, these tests are not failing with or without my change. We have been able to repro it from spark, and its apparent from the linked PR that this was missed. That said I do think we should add a test or figure out why it isn't working. I tried setting |
I think it is hard to reproduce a test that "fails" because this is a memory access UB issue rather than a validity issue. |
/ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Piling on to the 👍s here. Good catch, @abellina.
/merge |
Fixes #16706
I'll build/test our stack with this change, but it looks like a typo.
If there's a quick unit test we can add I'd be happy to hear recommendations or for someone else to follow on with such a test.