zeroshade commented on code in PR #1124:
URL: https://github.com/apache/arrow-go/pull/1124#discussion_r3760311309
##########
parquet/pqarrow/file_reader.go:
##########
@@ -219,6 +219,13 @@ func (fr *FileReader) allRowGroupFactory() itrFactory {
//
// IncludedLeaves and RowGroups are used to specify precisely which leaf
indexes and row groups to read a subset of.
func (fr *FileReader) GetFieldReader(ctx context.Context, i int,
includedLeaves map[int]bool, rowGroups []int) (*ColumnReader, error) {
+ if i < 0 || i >= len(fr.Manifest.Fields) {
+ return nil, fmt.Errorf("%w: invalid field index chosen %d,
there are only %d fields", arrow.ErrIndex, i, len(fr.Manifest.Fields))
Review Comment:
This establishes `errors.Is(err, arrow.ErrIndex)` for `GetFieldReader`, but
the identical bounds check in `getColumnReader` still returns a plain
`fmt.Errorf`. That leaves `GetColumn` and `RowGroup(...).Column(...).Read` as
the adjacent reader-index paths that cannot be classified with `errors.Is`.
Non-blocking for the panic fix, but could we wrap `arrow.ErrIndex` there too
and add one assertion so the typed index-error contract is consistent?
--
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]
