List Auxiliary Buffer NullMask Fix #3156
Conversation
| @@ -52,7 +52,9 @@ void ListAuxiliaryBuffer::resizeDataVector(ValueVector* dataVector) { | |||
| auto buffer = std::make_unique<uint8_t[]>(capacity * dataVector->getNumBytesPerValue()); | |||
| memcpy(buffer.get(), dataVector->valueBuffer.get(), size * dataVector->getNumBytesPerValue()); | |||
| dataVector->valueBuffer = std::move(buffer); | |||
| dataVector->nullMask->resize(capacity); // note: allocating 64 times what is needed | |||
| dataVector->nullMask->resize( | |||
There was a problem hiding this comment.
can u check if we are making the same mistake in other places when resizing nullMask? If so, this logic should be hidden inside NullMask class it self.
There was a problem hiding this comment.
I ran "Find All References" in VSCode and the only result that came up was this instance. The function is probably only used here.
More generally, the main problem is that NullMask refers to capacity in the int64 sense, rather than in the bitmap sense. For example, the capacity passed to the constructor specifies its size in int64 chunks, so it would make more sense to change everything to bitmap.
There was a problem hiding this comment.
I don't think int64 chunks or vector matters on this problem. We should encapsulate the details inside NullMask, so the best way is to modify resize function accordingly.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3156 +/- ##
=======================================
Coverage 92.08% 92.08%
=======================================
Files 1168 1168
Lines 44065 44065
=======================================
Hits 40577 40577
Misses 3488 3488 ☔ View full report in Codecov by Sentry. |
The List Auxiliary Buffer allocates 64 times the necessary amount of null bitmap memory when resizing the buffer. This one-line change was made in its own PR so as to be easy to revert.