Skip to content

Conversation

@siddharthteotia
Copy link
Contributor

@elahrvivaz , @StevenMPhillips

Reverting the changes made for ARROW-886 -- #591

(1) Don't explicitly reallocate the offsetVector in realloc() function of Variable Length Vectors. 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.

(2) Doing (1) will break the unit test added as part of PR 591 so we need to remove that as well.

Copy link
Contributor

@elahrvivaz elahrvivaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly confused about what the use-case for reAlloc is if we still have to use setSafe - I guess reAlloc is meant to be called internally from setSafe? anyway, I don't think this change will affect our application, so it's ok with me.

@wesm
Copy link
Member

wesm commented Aug 3, 2017

Is there any way to assert the correct behavior in a unit test?

@siddharthteotia
Copy link
Contributor Author

Looks like we have realloc related unit tests in TestVectorRealloc.java. But as a follow-up, I am planning to revisit realloc related tests and add to/enhance them.

@wesm
Copy link
Member

wesm commented Aug 3, 2017

OK, no problem

@asfgit asfgit closed this in 1874a8b Aug 3, 2017
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@elahrvivaz , @StevenMPhillips

Reverting the changes made for ARROW-886 -- apache#591

(1) Don't explicitly reallocate the offsetVector in realloc() function of Variable Length Vectors.   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.

(2) Doing (1) will break the unit test added as part of PR 591 so we need to remove that as well.

Author: siddharth <siddharth@dremio.com>

Closes apache#937 from siddharthteotia/ARROW-1310 and squashes the following commits:

c5a2707 [siddharth] ARROW-1310: revert changes made in ARROW-886
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants