tanmayrauth commented on code in PR #1585:
URL: https://github.com/apache/iceberg-go/pull/1585#discussion_r3678484616
##########
literals.go:
##########
@@ -193,13 +193,16 @@ func LiteralFromBytes(typ Type, data []byte) (Literal,
error) {
return GeoLiteral{val: data, typ: typ}, nil
case FixedType:
- if len(data) != t.Len() {
+ if len(data) < t.Len() {
// looks like some writers will write a prefix of the
fixed length
// for lower/upper bounds instead of the full length.
so let's pad
// it out to the full length if unpacking a fixed
length literal
padded := make([]byte, t.Len())
copy(padded, data)
data = padded
+ } else if len(data) > t.Len() {
+ return nil, fmt.Errorf("%w: fixed[%d] value has %d
bytes",
Review Comment:
Worth flagging the failure-mode change this introduces: the manifest
evaluators panic(err) on any LiteralFromBytes error (evaluators.go:172, 182,
and ~34 other call sites), so an oversized fixed bound now crashes the scan
instead of silently mis-pruning. That's a net improvement — a loud panic beats
silent wrong results — and the panic(err) is pre-existing and shared by every
error branch, so not something to fix here. But since the comment right above
concedes non-compliant writers exist (they write short prefixes), a writer
emitting oversized fixed bounds would now hard-crash pruning. If graceful
degradation is preferred, the real fix is at the call sites: treat a
bound-parse error as rowsMightMatch rather than panicking. Fine to leave that
out of this PR — just make sure the crash-on-malformed-bound behavior is
intended.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
