zeroshade commented on code in PR #1032:
URL: https://github.com/apache/arrow-go/pull/1032#discussion_r3660790017
##########
parquet/metadata/page_index.go:
##########
@@ -244,6 +244,24 @@ func NewOffsetIndex(serializedIndex []byte, _
*parquet.ReaderProperties, decrypt
return &offsetIndex
}
+func deserializeColumnIndex(descr *schema.Column, serializedIndex []byte,
props *parquet.ReaderProperties) (idx ColumnIndex, err error) {
+ defer func() {
+ if recovered := recover(); recovered != nil {
+ err = fmt.Errorf("%w: invalid column index: %v",
arrow.ErrInvalid, recovered)
+ }
+ }()
+ return NewColumnIndex(descr, serializedIndex, props, nil), nil
Review Comment:
Two asks on this recover block (same applies to `deserializeOffsetIndex`
below).
**`%v` discards the cause.** Prefer the repo's own
`utils.FormatRecoveredError` (`internal/utils/recovery.go:26-31`), which wraps
with `%w`. Right now `errors.Is(err, arrow.ErrInvalid)` works but there's no
way to reach the underlying `*thrift.tProtocolException`.
**The scope catches genuine bugs too.** This wraps the whole constructor —
decrypt, thrift decode, type switch, `newTypedColumnIndex` validation, and
per-page min/max decoding — and never re-panics. I verified that a nil `descr`
surfaces as `invalid: invalid column index: runtime error: invalid memory
address or nil pointer dereference`, i.e. an arrow-go nil-deref reported as a
corrupt user file. Re-panicking on `runtime.Error`, or narrowing the recover to
just the deserialization call, would keep programmer errors distinguishable
from bad input.
--
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]
