feat: Add DecodedVector::sharedBase() #12249
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D69081492 |
facebook-github-bot
added
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
Yuhta
force-pushed
the
export-D69081492
branch
from
cb18bfe to
c8de194
Compare
Summary: Sometimes we need to take shared ownership of the base value vector of a dictionary. The current `DecodedVector` only keeps reference to a raw pointer so there is no way to get hold of the `shared_ptr`. We add `DecodedVector::sharedBase()` and overload of `DecodedVector::decode` to take `shared_ptr` so that we can get the shared ownership. Differential Revision: D69081492
|
This pull request was exported from Phabricator. Differential Revision: D69081492 |
Summary: Sometimes we need to take shared ownership of the base value vector of a dictionary. The current `DecodedVector` only keeps reference to a raw pointer so there is no way to get hold of the `shared_ptr`. We add `DecodedVector::sharedBase()` and overload of `DecodedVector::decode` to take `shared_ptr` so that we can get the shared ownership. Differential Revision: D69081492
Yuhta
force-pushed
the
export-D69081492
branch
from
c8de194 to
22ad3c0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69081492 |
| vector_size_t size) const { | ||
| VELOX_CHECK(!isIdentityMapping_); |
There was a problem hiding this comment.
do we plan to use dictionaryWrapping() when these conditions are not met?
Edit: Just noticed you added a test for these case, curious why we would want to use this API for these cases as it would end up creating inefficient dictionaries. Is it because we plan to add nulls on top of that?
Also, can you update the method comment to remove the part about Requires /// isIdentityMapping() == false and isConstantMapping() == false. if we plan to keep this?
There was a problem hiding this comment.
I added these to support merging DICT + FLAT, DICT + CONST, CONST + CONST. It might not be the most efficient way to do this; I will revisit this if we need to avoid allocating the indices and nulls too early, but for now it is working for the internal use case. As far as this method is concerned, it does not hurt to enable these as there are very clear flags we can check if we want to forbid them on call site.
Summary: Sometimes we need to take shared ownership of the base value vector of a dictionary. The current `DecodedVector` only keeps reference to a raw pointer so there is no way to get hold of the `shared_ptr`. We add `DecodedVector::sharedBase()` and overload of `DecodedVector::decode` to take `shared_ptr` so that we can get the shared ownership. Reviewed By: bikramSingh91 Differential Revision: D69081492
Summary: Sometimes we need to take shared ownership of the base value vector of a dictionary. The current `DecodedVector` only keeps reference to a raw pointer so there is no way to get hold of the `shared_ptr`. We add `DecodedVector::sharedBase()` and overload of `DecodedVector::decode` to take `shared_ptr` so that we can get the shared ownership. Reviewed By: bikramSingh91 Differential Revision: D69081492
Yuhta
force-pushed
the
export-D69081492
branch
2 times, most recently
from
be0b593 to
226cee6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D69081492 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D69081492 |
|
This pull request has been merged in 8e6e2cc. |
Summary: Sometimes we need to take shared ownership of the base value vector of a dictionary. The current
DecodedVectoronly keeps reference to a raw pointer so there is no way to get hold of theshared_ptr. We addDecodedVector::sharedBase()and overload ofDecodedVector::decodeto takeshared_ptrso that we can get the shared ownership.Differential Revision: D69081492