Vector isWritable/Reusable/Recyclable APIs

[kevinwilfong]

This PR #2232 adds a new isWritable API to BaseVector.

There are already isRecyclable and isReusableFlatVector that are in the same spirit, but with stricter requirements designed for different use cases.

Summarizing the 3 APIs:

isWritable

• Checks:
  • The vector is singly referenced.
  • The vector has a Flat-like encoding (Flat, Array, Map, Row).
  • Any non-null child Buffers are mutable and singly referenced (excluding StringBuffers).
  • All of these conditions hold for child Vectors recursively.
• Use case:
  • Checking if ensureWritable has already been called on the Vector.

isReusableFlatVector

• Checks:
  • The vector is singly referenced.
  • The vector is Flat.
  • Any non-null child Buffers are mutable and singly referenced (for Flat Vectors this is just the null and values Buffers).
  • The StringBuffers are uniquely referenced.
• Use case:
  • Checking if an input Vector can be reused as an output Vector in the SimpleFunctionAdapter.

isRecyclable

• Checks:
  • The vector is Flat.
  • The nulls Buffer is itself null or unique.
  • The values Buffer is not null and unique.
• Use case:
  • Checking if a Vector can be returned to the VectorPool.

Comparing isWritable and isReusableFlatVector, they are very close, isReusableFlatVector just has stricter checks on the encoding (only Flat) and the StringBuffers being unique (I'm unclear on why this is needed). It's easy to see how isReusableFlatVector could be rewritten to use isWritable, or the 5 or so calls could be modified to check isWritable, the encoding, and the StringBuffers are unique, or a helper function outside BaseVector could be written to keep these all in sync but outside the Vector API.

Comparing isWritable and isRecyclable they're more different. Is the values Buffer always non-null? Is the fact it allows the Buffers to not be mutable() an oversight? if the answer to both of these is yes, it's again straightforward to modify the only place this function is used to call isWritable instead and apply the stricter conditions there removing it from the Vector API.

[kevinwilfong]

Regarding the naming, I like isWritable better than isReusable or isRecyclable because the latter imply we're repurposing a Vector for a new use, however, in the use case I have in mind, we're actively building a Vector and just checking that ensureWritable was called at some point earlier in the process. In the cases of isReusableFlatVector and isRecyclable, we're also trying to avoid calling ensureWritable to avoid allocating more a new Vector, so it makes sense to me semantically.

[kevinwilfong]

Is the values Buffer always non-null?

This is not true, but prepareForReuse() which is called shortly after in VectorPool could set it to null if it's not mutable (which again isn't checked).

[kevinwilfong]

Illustrating the differences as a table:

             | encodings             | vector singly referenced | child Buffers                                                 | StringBuffers | recursive
isWritable   | Flat, Array, Map, Row | yes                      | null or (mutable and unique)                                  | no check      | yes
isReusable   | Flat                  | yes                      | null or (mutable and unique)                                  | unique        | N/A
isRecyclable | Flat                  | not checked              | (nulls is null or unique) and (values is non-null and unique) | no check      | N/A

[kevinwilfong]

It's easy to see how isReusableFlatVector could be rewritten to use isWritable, or the 5 or so calls could be modified to check isWritable, the encoding, and the StringBuffers are unique, or a helper function outside BaseVector could be written to keep these all in sync but outside the Vector API.

It doesn't look like those 5 calls or closely related (2 are but they're adjacent) so having a single function vs. just checking both isWritable and isFlat isn't needed to maintain consistency. As long as the StringBuffers check isn't needed.

[mbasmanova]

@kevinwilfong Thank you for putting this together. Logically, it seems to me that we need only one method that checks if the vector is writable, e.g. recursively singly referenced. There are use cases where we want to only allow for writable vectors of primitive types. In these cases the caller could use isFlatEncoding() && . Hence, I wonder if we can consolidate all these methods into just one with the behavior of the new isWritable.

[kevinwilfong]

Yep, that sounds much cleaner. I'll update the PR.

[Yuhta]

Looks like in some places we are loosening the condition to allow non-singly referenced vector/buffers to be mutated. One of the example:

velox/velox/dwio/dwrf/reader/ColumnReader.h

Line 163 in 897b2c1

const bool reset = buffer->refCount() > 2;

[kevinwilfong]

It looks like the StringBuffers check was pulled out of SimpleFunctionAdapter, I'll move it back as it only looks like it's needed there #1091