Skip to content

Conversation

@amansinha100
Copy link

…ating pruning vector (other types were already using setSafe).

@sfc-gh-zfong
Copy link

Is there a small unit test case that reproduces this problem?

@mehant
Copy link
Contributor

mehant commented Oct 19, 2015

+1.

Changes look fine to me, given that all the other types use setSafe() it makes sense to use the same method for DATE, TIMESTAMP for consistency. However I feel that we might be addressing a symptom here, do you have any thoughts on what the actual issue might be?

@amansinha100
Copy link
Author

@mehant , yes the underlying issue is simple: the null bit vector for the NullableDateVector is allocated at 4096 bytes (actually, the UInt1Vector is used for this, hence the error message shows length: 1). The 4096 is based on the static constant BaseValueVector.INITIAL_VALUE_ALLOCATION. Since set() does not do any reallocation, once this limit is reached, attempt to write to this vector causes IOBE. setSafe() does realloc()s. I am pretty sure I can create a test case with > 4096 files partitioned on date column that will repro this issue. I will need to work with QA/performance folks on creating this test.

@amansinha100
Copy link
Author

@zfong, please see my last explanation about why the repro does not occur at small scale. Hence, adding a unit test won't help ... unless the static constant that controls the initial size of the value vectors is made configurable...but that would require more code changes.

…ating pruning vector (other types were already using setSafe).

close apache#208
@asfgit asfgit merged commit f1100a7 into apache:master Oct 19, 2015
@sfc-gh-zfong
Copy link

Aman -- yup, saw that. Thanks.

On Mon, Oct 19, 2015 at 8:57 AM, Aman Sinha notifications@github.com
wrote:

@zfong https://github.com/zfong, please see my last explanation about
why the repro does not occur at small scale. Hence, adding a unit test
won't help ... unless the static constant that controls the initial size of
the value vectors is made configurable...but that would require more code
changes.


Reply to this email directly or view it on GitHub
#208 (comment).

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.

4 participants