-
Notifications
You must be signed in to change notification settings - Fork 915
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
Fix memory write error in get_list_child_to_list_row_mapping utility #8994
Fix memory write error in get_list_child_to_list_row_mapping utility #8994
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #8994 +/- ##
===============================================
Coverage ? 10.71%
===============================================
Files ? 114
Lines ? 19090
Branches ? 0
===============================================
Hits ? 2046
Misses ? 17044
Partials ? 0 Continue to review full report at Codecov.
|
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.
CMake changes LGTM
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.
Kudos for moving the non-template functions into the .cu
. +1.
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.
Look good.
@gpucibot merge |
Reference issue #8883 and depends on fixes in PR #8884
The
get_list_child_to_list_row_mapping
builds a map for rolling operation on a lists column. In thethrust::scatter
call a map value includes the last offset which will always be out-of-bounds to given output vector. This output vector is used to build the resultant output map by callingthrust::inclusive_scan
but the out-of-bounds offset value is not used -- which is why the utility does not fail. The fix in this PR simply allocates an extra row in the intermediate vector so thethrust::scatter
will not write to out-of-bounds memory. Since the value is eventually ignored, it does not effect the result.The code in this function was creating many temporary columns incorrectly using the passed in
device_resource_manager
variablemr
. The code was corrected by changing these to be justdevice_uvector's
instead making it more clear that these are internal temporary memory buffers. Further the code callingget_list_child_to_list_row_mapping
utility is using the output as a temporary column and so this PR fixes the logic to correct the memory resource usage.