Add row conversion code from spark-rapids-jni #14664
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
ttnghia
added
3 - Ready for Review
hyperbolic2346
approved these changes
Dec 20, 2023
Signed-off-by: Nghia Truong <[email protected]>
vyasr
approved these changes
Dec 20, 2023
cpp/include/cudf/row_conversion.hpp
Outdated
There was a problem hiding this comment.
Looks like this is just copied/moved from java/src/main/native/src/row_conversion.hpp
There was a problem hiding this comment.
There was a problem hiding this comment.
The style diff here is because the code in java/ was not formatted with clang-format.
There was a problem hiding this comment.
That accounts for the style diffs. There were material differences in the spark-rapids-jni version of row_conversion.cu that's not the easiest to spot in this diff. This remains a temp fix, of course, until the CCCL issue is resolved.
mythrocks
approved these changes
Dec 20, 2023
Signed-off-by: Nghia Truong <[email protected]>
mythrocks
reviewed
Dec 21, 2023
|
|
||
| namespace cudf { | ||
| namespace jni { |
There was a problem hiding this comment.
I admire your thoroughness in this, @ttnghia.
|
Seems there was a cmake file missed |
Signed-off-by: Nghia Truong <[email protected]>
|
/merge |
This was referenced Dec 21, 2023
3 tasks
rapids-bot bot
pushed a commit
that referenced
this pull request
Jan 4, 2024
A call to a deprecated `cudf::make_strings_column` factory function was introduced in #14664. This call had been previously fixed in #14461 but was lost when the source file was moved in #14664. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #14695
copy-pr-bot bot
pushed a commit
that referenced
this pull request
Mar 5, 2024
This reverts commit 36f56c9. # Conflicts: # cpp/include/cudf/row_conversion.hpp # cpp/tests/transform/row_conversion.cpp # java/src/main/native/src/row_conversion.cu
rapids-bot bot
pushed a commit
that referenced
this pull request
Mar 5, 2024
This is to remove the row conversion code from libcudf. It was move from spark-rapids-jni (by #14664) to temporarily workaround the issue due to conflict of kernel names that causes invalid memory access when calling to `thrust::in(ex)clusive_scan` (NVIDIA/spark-rapids-jni#1567). Now we have fixes for the namespace visibility issue (by marking all libcudf kenels private in rapidsai/rapids-cmake#523 and NVIDIA/cuCollections#422) and need to move back the code. Closes #14853. Authors: - Nghia Truong (https://github.com/ttnghia) Approvers: - David Wendt (https://github.com/davidwendt) - Bradley Dice (https://github.com/bdice) URL: #15234
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This temporarily moves the row conversion code from spark-rapids-jni into libcudf. It is necessary to have the row conversion code compiled in a static library to overcome a CCCL issue that triggers invalid memory access when calling to
thrust::in(ex)clusive_scan(NVIDIA/spark-rapids-jni#1567).This also removes the existing (outdated)
row_conversioncode under thejavafolder in cudf repository.In the future, when we have CCCL updated to fix the issue (1567), we may need to move the code back into spark-rapids-jni.