Add get_element for struct column
#8578
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8578 +/- ##
===============================================
Coverage ? 83.01%
===============================================
Files ? 109
Lines ? 18225
Branches ? 0
===============================================
Hits ? 15129
Misses ? 3096
Partials ? 0 Continue to review full report at Codecov.
|
isVoid
added
feature request
cpp/src/copying/get_element.cu
Outdated
| auto row_contents = | ||
| std::make_unique<column>(slice(input, index, index + 1), stream, mr)->release(); | ||
| auto scalar_contents = std::make_unique<table>(std::move(row_contents.children)); | ||
| return std::make_unique<struct_scalar>(std::move(*scalar_contents), valid, stream, mr); |
There was a problem hiding this comment.
I think std::move(*scalar_contents) is wrong as the unique_ptr hasn't been informed that it doesn't have ownership anymore. I think you want:
auto scalar_contents = table(std::move(row_contents.children));
std::make_unique<struct_scalar>(std::move(scalar_contents), valid, stream, mr);
There was a problem hiding this comment.
std::make_unique<struct_scalar>(std::move(scalar_contents), valid, stream, mr);
Not quite. The struct_scalar constructor needs a table&&.
I think std::move(*scalar_contents) is wrong as the unique_ptr hasn't been informed that it doesn't have ownership anymore.
The unique_ptr retains ownership of the "moved-from" object, which should be safe to destroy when the unique_ptr goes out of scope. (Please correct me if I'm wrong.)
There was a problem hiding this comment.
I believe you are correct. To clarify, the object (the table) being pointed to by the std::unique_ptr<> has had it's internal buffers emptied out, but the table held by the pointer itself still exists, so when it gets deleted, it's destructor does nothing.
It is a little bit subtle though. @robertmaynard 's suggestion makes the sequence of operations a lot more clear.
As a side note, it's a little surprising the PR that added these move constructors didn't actually have any tests for them.
There was a problem hiding this comment.
Ah, I misread @robertmaynard's suggestion: scalar_contents is a table, in his case.
I agree. @robertmaynard's way reads better.
There was a problem hiding this comment.
Thanks for the comments. Moving to @robertmaynard 's suggestion.
From a lower level perspective, previously
std::make_unique<struct_scalar>(std::move(*scalar_contents), valid, stream, mr);
creates a table object with the contents that was pointed to by the unique_ptr and was later moved to the struct_scalar. After this step, the unique_ptr still points to a table object, but as a result of default move constructor, that object now contains 0 columns, effectively empty. It's safe for unique_ptr to deallocate this object because the table object is still live, its internal _columns field is also live, only it's empty.
Agreed it's subtle compared to the new writings.
Co-authored-by: nvdbaranec <[email protected]>
|
rerun tests |
isVoid
added
5 - Ready to Merge
|
@gpucibot merge |
Implements necessary `__getitem__` changes for python side. Partly addresses #8558, merge after #8578 Authors: - https://github.com/shaneding Approvers: - https://github.com/brandon-b-miller - Ashwin Srinath (https://github.com/shwina) URL: #8577
Partly addresses #8558
This PR supports retrieving a
struct_scalarfromSTRUCTtype column.