jbonofre commented on code in PR #1262:
URL: https://github.com/apache/arrow-java/pull/1262#discussion_r4186941500
##########
vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java:
##########
@@ -165,7 +165,7 @@ static void retrieveIndexVector(
for (int i = start; i < end; i++) {
if (!indices.isNull(i)) {
int indexAsInt = (int) indices.getValueAsLong(i);
- if (indexAsInt > dictionaryCount) {
+ if (indexAsInt < 0 || indexAsInt >= dictionaryCount) {
Review Comment:
This helper has a caller that doesn't pass a dictionary size.
`StructSubfieldEncoder.decode` calls:
```java
DictionaryEncoder.retrieveIndexVector(indices, transfer, valueCount, 0,
valueCount);
```
where `valueCount` is the struct's row count. With `>=` that gives two
problems:
- A 1-row struct whose `f0` child holds index 1 into a 2-entry dictionary
`["aa", "bb"]` decodes on main and now throws `Provided dictionary does not
contain value for index 1`.
- A 40-row struct with index 39 into a 1-entry dictionary passes `39 >= 40`
and reaches `copyValueSafe`. For a fixed-width dictionary that ends in a raw
`MemoryUtil.copyMemory` in `BaseFixedWidthVector.copyFrom`.
Could you pass `dictionary.getVector().getValueCount()` in
`StructSubfieldEncoder` instead? The PR description says the struct decoder is
covered by this change, which only holds once that bound is fixed.
##########
vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java:
##########
@@ -165,7 +165,7 @@ static void retrieveIndexVector(
for (int i = start; i < end; i++) {
if (!indices.isNull(i)) {
int indexAsInt = (int) indices.getValueAsLong(i);
- if (indexAsInt > dictionaryCount) {
+ if (indexAsInt < 0 || indexAsInt >= dictionaryCount) {
throw new IllegalArgumentException(
"Provided dictionary does not contain value for index " +
indexAsInt);
}
Review Comment:
The narrowing to `int` happens before the range check, so a 64-bit index can
wrap into range. With a `BigIntVector` index, `4294967296` becomes `0` and
`-4294967295` becomes `1`, and both are accepted. A `UInt4` index of
`4294967295` is rejected, but the message reports `index -1`.
Checking the `long` first fixes both:
```suggestion
long index = indices.getValueAsLong(i);
if (index < 0 || index >= dictionaryCount) {
throw new IllegalArgumentException(
"Provided dictionary does not contain value for index " +
index);
}
int indexAsInt = (int) index;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
