Refactor multibyte_split output_builder
#11945
Conversation
upsj
added
3 - Ready for Review
Codecov ReportBase: 87.40% // Head: 88.16% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #11945 +/- ##
================================================
+ Coverage 87.40% 88.16% +0.75%
================================================
Files 133 133
Lines 21833 21977 +144
================================================
+ Hits 19084 19375 +291
+ Misses 2749 2602 -147
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
upsj
force-pushed
the
refactor/output_builder_generalize
branch
from
f408fab to
ff3f1b9
Compare
upsj
added
cuIO
| */ | ||
| void advance_output(size_type actual_size, rmm::cuda_stream_view stream) | ||
| { | ||
| CUDF_EXPECTS(actual_size <= _max_write_size, "Internal error"); |
There was a problem hiding this comment.
This is more of an external error, isn't it?
There was a problem hiding this comment.
External to the data structure, yes, but internal to cuDF. Originally this was an assertion, but I changed it to also fail in release builds.
There was a problem hiding this comment.
I saw this was copied. I'm somewhat torn here. I don't like a message like this with no real information about what went wrong, but I also don't see it as a big deal since CUDF_EXPECTS will output file and line number information so it is easy to find out where the error lives.
There was a problem hiding this comment.
My thought here was that any kind of error message will probably end up with the user, and they will not really be able to make much sense of it either way (nondescript vs. descriptive), let alone do anything against it, since this would point to a bug, not user error.
Co-authored-by: Mike Wilson <[email protected]>
upsj
added
5 - Ready to Merge
|
rerun tests |
|
@gpucibot merge |
Description
This PR moves the
output_builderandsplit_device_spanclasses out ofmultibyte_splitand adds an iterator for thesplit_device_span, enabling it to be used directly in Thrust algorithms.I also included a fix from #11875 to make the integration easier once that is merged.
Checklist