fix(parquet/pqarrow): validate reader indexes - #1124
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The bounds checks are correctly placed before the direct field and row-group index accesses, converting previously possible panics into typed errors while preserving valid reads.
The focused regression and complete parquet/pqarrow package suite pass locally on head d9988c9afc85. One small, non-blocking consistency observation is included inline. RC and Test CI are still running.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. The findings below are observations, not blockers; an Apache Arrow Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.
zeroshade
left a comment
There was a problem hiding this comment.
The follow-up wraps the remaining column-reader bounds error with arrow.ErrIndex and adds matching regression coverage, resolving the prior consistency observation.
Re-review at 10ddd13aa2d7: the focused test passed 50 repetitions, the complete parquet/pqarrow package passed, and race plus 32-bit runs passed. All required CI checks are green.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer.
Rationale for this change
Some Arrow Parquet reader paths index the field manifest or row-group metadata directly. Invalid input can panic even though the surrounding APIs return errors.
What changes are included in this PR?
Validate field indexes in GetFieldReader and row-group indexes in GetFieldReader and ReadColumn before using them.
Are these changes tested?
go test ./parquet/pqarrow -run TestFileReaderIndexValidationAre there any user-facing changes?
Invalid field and row-group indexes now return errors instead of panicking.