DictionaryVector::flatRawNulls() method is not thread safe #2202
mbasmanova
started this conversation in
General
Replies: 3 comments 1 reply
-
This is a problem:
The member function is announced read-only, but it is NOT const in the code, thus allowing modification of the members w/o engineers noticing it. |
Beta Was this translation helpful? Give feedback.
0 replies
-
+1. flatNullsBuffer() not being const is counter-intuitive and error-prone. Good to fix if we don't have many users relying on this API yet. |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Velox vectors are designed to be safe to share across threads for read-only access. DictionaryVector::flatRawNulls() is a read-only method, but it is not thread-safe as it lazily allocates and populates flatNullsBuffer_.
The proposal is to remove this method and replace its usage with DecodedVector::nulls() method.
Further, to make it easier to reason about DecodedVector API, we propose to remove nullIndices() method and modify nulls method to return a nullptr if there are no nulls or a nulls buffer that can be accessed using top-level row numbers. This would make DecodedVector::nulls() equivalent to current BaseVector::flatRawNulls().
Currently, DecodedVector::nulls() returns either (1) a nullptr (when there are no nulls), (2) a nulls buffer from the base vector if wrappings do not add nulls or (3) a combined nulls buffer that represents a merge of the nulls from the base and all the wrappings. DecodedVector::nullIndices() method allows to distinguish between 2nd and 3rd case by returning indices() buffer in case of (2) and nullptr in case of (3).
DecodedVector is not thread-safe and each thread is expected to make and use its own instance.
CC: @pedroerp @oerling @spershin
Beta Was this translation helpful? Give feedback.
All reactions