VariableLengthVectors don't reAlloc offsets

[asfimport]

Reporter: Emilio Lahr-Vivaz / @elahrvivaz
Assignee: Emilio Lahr-Vivaz / @elahrvivaz

Related issues:

• Arrow 0.3 release (blocks)

_{Note: This issue was originally created as ARROW-886. Please see the migration documentation for further details.}

[asfimport]

Wes McKinney / @wesm:
Issue resolved by pull request 591
#591

[asfimport]

Steven Phillips / @StevenMPhillips:
I'm just now noticing this, but this change (along with ARROW-815) actually breaks the way that reAlloc was designed to work. It was originally meant to be an internal method, which was used by the various setSafe() methods. The idea was that a user of the ValueVector could call setSafe() instead of set(), and this would automatically and transparently handle the expansion of the underlying buffer in the event that we reached the end of the currently allocated buffer.

For variable length vectors, the re-allocation of the data buffer should be completely independent of the re-allocation of the offset vector's buffer. But this change causes reAlloc() to be called on the offset vector anytime reAlloc() is called on the parent vector.

[asfimport]

Emilio Lahr-Vivaz / @elahrvivaz:
I forget exactly what scenario led to this being an issue, but I think using the writer framework, possibly with FixedSizeListVectors (which aren't accessible through writers), caused the vector to run out of space. Possibly instead of the changes that were made to reAlloc, we instead need to make sure setSafe() is being called appropriately, and add it where needed.

[asfimport]

Steven Phillips / @StevenMPhillips:
Do you have any external code that is using the reAlloc() method explicitly, since ARROW-815 made that method public?

[asfimport]

Emilio Lahr-Vivaz / @elahrvivaz:
We are calling it explicitly. In our use case we have a NullableMapVector with a bunch of child vectors of varying types. Currently we just check the size of the top-level vector and call re-alloc if we need to grow. For variable length child vectors, we call setSafe - however, for fixed size vectors it would be nice to be able to just do the check once on the MapVector, then expand as needed, instead of using setSafe and checking on each vector. We're trying to minimize reads and checks during write.

[asfimport]

Emilio Lahr-Vivaz / @elahrvivaz:
Is the only issue with variable length buffers? Possibly in those classes we could just not reAlloc the data buffer, and leverage setSafe for that.

[asfimport]

Steven Phillips / @StevenMPhillips:
The original idea was that a user of this interface would know in advance how many records they intended to write in a given batch, but wouldn't necessarily know the total length of the variable length columns. So a user would use the setInitialCapacity() method. Then they could use set() for the fixed length vectors, and setSafe() for the variable length vectors.

Also, we could add a new method to the valuevector interface, such as expandValueCapacity(int values) which would allow expanding the value capacity of the vectors (including child vectors) but wouldn't touch the variable length data buffer.

[asfimport]

Siddharth Teotia / @siddharthteotia:
@elahrvivaz,

Just picking up the conversation. Are you fine with the following changes?

(1) Make realloc() back to being an internal method

(2) Don't do explicit re-allocation of the offsetVector() from the realloc() function in VariableLengthVectors.java. If we call setSafe() on variable length vector, it will internally invoke setSafe() on the corresponding offsetVector and the latter function can decide whether to reallocate the offsetVector or not.

(3) Finally, as Steven suggested, we can add a new method to the ValueVector interface that can allow growing the offsetVector() independent of calling realloc() and growing the variable length data buffer.

Thanks,
Siddharth

[asfimport]

Emilio Lahr-Vivaz / @elahrvivaz:
That would probably be fine. My use case is:

1. Create a parent vector (NullableMapVector) with several children of different types (some fixed width, some variable width)
2. Write an unknown number of elements to the vectors, using the child vector mutators

For the variable-width vectors, I'm using mutator.setSafe, as there is no other way to guarantee sufficient size (to my knowledge). But for the fixed with vectors, I'd like to check size once on the parent, then expand them all as needed, instead of having to use .setSafe for every write. Having a way to externally grow a vector (outside of setSafe) seems like it would be useful any time you don't know the number of elements you're writing up front.

[asfimport]

Siddharth Teotia / @siddharthteotia:
@elahrvivaz,

As far as the commit #591 is concerned, are you fine with undoing the changes?

(1) Don't explicitly reallocate the offsetVector in realloc() function of Variable Length Vectors.
(2) Doing (1) will break the unit test added as part of PR 591 so we need to remove that as well.

I have created a PR for the above two items – #937 this basically reverts your change for the above items.

Thanks,
Siddharth