DictionaryVector::flatRawNulls() method is not thread safe

[mbasmanova]

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

[spershin]

This is a problem:

DictionaryVector::flatRawNulls() is a read-only method, but it is not thread-safe as it lazily allocates and populates flatNullsBuffer_.

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.

[pedroerp]

+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.

[mbasmanova]

Here is the existing usage of flatRawNulls. It is quite a few places, but should be manageable.

• deselectRowsWithNulls in OperatorUtils used in

  • GroupingSet::addInputForActiveRows
  • HashBuild::addInput
  • HashProbe::addInput

• PartitionedOutput::collectNullRows

• CastExpr

• CoalesceExpr

• Expr.cpp

• prestosql/IsNullFunction

• sparksql/CompareFunctionsNullSafe

• sparksql/Hash

• sparksql/LeastGreatest

• implementations in Dict and Lazy vectors

• VectorTest

[mbasmanova]

#2205 modified DecodedVector API.

#2203, #2208, #2210, #2211, #2213, #2214 replaced usage of flatRawNulls with DecodedVector::nulls.

#2218 removed the flatRawNull method.