fix(scalar): validate dictionary indices before conversion#927
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Nice — validating the physical index in its native integer domain before converting to int (physicalIndex) covers the full signed/unsigned int8–int64 set, so wrapped/negative indices no longer reach GetScalar. Routing both ValidateFull and GetEncodedValue through it is clean, and the test matrix (incl. MaxUint64, negatives) is thorough. Two small nits inline.
| } | ||
| return int(idx), nil | ||
| default: | ||
| return 0, fmt.Errorf("unimplemented dictionary type %s", s.DataType()) |
There was a problem hiding this comment.
Nit: report the index type here (dt.IndexType) rather than the whole dictionary type, matching the old GetEncodedValue default. Also consider adding an empty-dictionary (len == 0) out-of-range case to TestDictionaryScalarIndexBounds.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LGTM. Centralizing the dictionary scalar index validation with thorough coverage (signed/unsigned/max/negative/out-of-bounds/empty-dictionary) looks good. Thanks!
2 participants
What
Validate dictionary indices in their native signed or unsigned domain before converting them to int. Use the checked physical index for GetEncodedValue as well, so malformed scalars return an error instead of wrapping on 32-bit or 64-bit platforms.
Tests
go test ./arrow/scalarGOOS=linux GOARCH=386 go test -c -o /dev/null ./arrow/scalarpre-commit run golangci-lint-full --all-filesThe broader
go test ./arrow/...run is otherwise green for the reached packages but the checkout lacks the CSV/parquet fixture directories.