GH-40347: [C#] ArrowReaderImplementation: fix for out of range exception#40355
GH-40347: [C#] ArrowReaderImplementation: fix for out of range exception#40355davidhcoe wants to merge 1 commit intoapache:mainapache/arrow:mainfrom davidhcoe:dev/out-of-range-fixdavidhcoe/arrow:dev/out-of-range-fixCopy head branch name to clipboard
Conversation
|
|
|
|
||
| arrays.Add(ArrowArrayFactory.BuildArray(arrayData)); | ||
| } while (recordBatchEnumerator.MoveNextNode()); | ||
| } while (recordBatchEnumerator.MoveNextNode() && schemaFieldIndex < schema.FieldsList.Count); |
There was a problem hiding this comment.
This seems reasonable however a few questions @davidhcoe -
- What happens to the state of recordBatchEnumerator if you haven't iterated through all the elements? Do you have to manually close it?
- Similarly, what happens if you exhaust the recordBatchEnumerator before reaching the field list count? Do fields beyond this get exposed correctly?
|
Hmm... I guess the conversations we previously had about this were over Teams and not on the pull request. That was clearly a mistake :(. I don't feel like I understand the details of the FlatBuffers -> ArrayData conversion well enough to be able to evaluate this change. I have similar concerns to those expressed by @jduo. I was never able to get a repro of the reported exception, so I couldn't even debug through the code to understand the consequences. |
@CurtHagenlocher , would it make sense to close this PR for now? We can always re-open it. |
|
@davidhcoe I know it's been a very long time; do you recall the circumstances under which you observed the error? If not, then I think we should close this for now. |
|
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer. |
|
Closing as stale |
Rationale for this change
Adds an additional check for processing the loop to ensure it doesn't go longer than the FieldsList in the schema.
What changes are included in this PR?
Are these changes tested?
All tests pass with the change included
Are there any user-facing changes?