GH-1098: [Java][Vector] Fix always-true precondition check in LargeListVector.setValueCount#1099
Conversation
…argeListVector.setValueCount
LuciferYang
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
This comment has been minimized.
This comment has been minimized.
|
Could a maintainer please add the bug-fix label to this PR? I don't have label permissions on this repository. Thank you! |
There was a problem hiding this comment.
Good one. I looked into this: the bug has been there since the class was created, see 4deba44#diff-c3066ea815144f3e05b277c87cae8a7701f0f1481e15da508261664ab017d944R984
The PR fixes the bug and has test coverage for all equivalence classes. The correct values are in the interval [0, Integer.MAX_Value]; there are tests for a negative value, 0, a positive value smaller than Integer.MAX_Value, Integer.MAX_Value and a positive value greater than Integer.MAX_Value.
Thank you
| */ | ||
| Preconditions.checkArgument( | ||
| childValueCount <= Integer.MAX_VALUE || childValueCount >= Integer.MIN_VALUE, | ||
| childValueCount <= Integer.MAX_VALUE && childValueCount >= 0, |
There was a problem hiding this comment.
nit: I'd use childValueCount >= 0 && childValueCount <= Integer.MAX_VALUE for the sake of readability.
| vector.setLastSet(0); | ||
|
|
||
| assertThrows( | ||
| IllegalArgumentException.class, |
There was a problem hiding this comment.
The error message throwing here is a bit confusing: IllegalArgument LargeListVector doesn't yet support 64-bit allocations: -1
| // The Preconditions check should accept this value (not throw IllegalArgumentException). | ||
| // The child vector may throw OversizedAllocationException due to memory limits, | ||
| // which is expected and unrelated to the precondition validation. | ||
| vector.getOffsetBuffer().setLong(LargeListVector.OFFSET_WIDTH, (long) Integer.MAX_VALUE); |
There was a problem hiding this comment.
nit: Casting 'Integer.MAX_VALUE' to 'long' is redundant
| // when trying to allocate Integer.MAX_VALUE elements — this is fine, | ||
| // the precondition check itself passed. | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Maybe this can be replaced with
try {
vector.setValueCount(1);
} catch (IllegalArgumentException e) {
fail("setValueCount should not reject childValueCount = Integer.MAX_VALUE", e);
} catch (OversizedAllocationException | OutOfMemoryException e) {
// OversizedAllocationException or other allocation errors are expected
// when trying to allocate Integer.MAX_VALUE elements — this is fine,
// the precondition check itself passed.
}
|
Thank you for the bug-fix @LuciferYang ! I've left some comments. My main concern is the confusing error message for small negative values. |
|
@axreldable Tbanks for your review.I am on vacation, I will provide fixes and feedback on your suggestions in 3 days |
4 participants
What's Changed
Fix the precondition check in
LargeListVector.setValueCount()where||made the condition always true (childValueCount <= Integer.MAX_VALUE || childValueCount >= Integer.MIN_VALUEis a tautology for anylongvalue), allowing silent data corruption whenchildValueCountexceedsInteger.MAX_VALUEor is negative.Changes:
||with&&andInteger.MIN_VALUEwith0to correctly boundchildValueCountto[0, Integer.MAX_VALUE], consistent withLargeListViewVectorchildValueCountInteger.MAX_VALUEvaluesCloses #1098.