Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Go] RecordReader has no way to propagate errors #33789

Closed
lidavidm opened this issue Jan 19, 2023 · 1 comment · Fixed by #33792
Closed

[Go] RecordReader has no way to propagate errors #33789

lidavidm opened this issue Jan 19, 2023 · 1 comment · Fixed by #33792
Assignees
Labels
Breaking Change Includes a breaking change to the API Component: Go Type: enhancement
Milestone

Comments

@lidavidm
Copy link
Member

Describe the enhancement requested

RecordReader's methods don't return err, so there's no way to propagate errors. For this reason, exported streams in the C Data Interface have no way of returning errors, either.

Changing the interface would of course be a breaking change. The alternative is to declare this:

type ClosableRecordReader interface {
        RecordReader
        Closable
}

which gives us one place to report errors (at the end).

Component(s)

Go

@zeroshade
Copy link
Member

At this point we might as well add the Err() error method to the array.RecordReader interface, this would allow us to potentially bring the ipc.Reader to be an array.RecordReader also without sacrificing the ability to retrieve errors.

lidavidm added a commit to lidavidm/arrow that referenced this issue Jan 19, 2023
@lidavidm lidavidm added the Breaking Change Includes a breaking change to the API label Jan 19, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jan 19, 2023
@zeroshade zeroshade added this to the 12.0.0 milestone Jan 19, 2023
zeroshade pushed a commit that referenced this issue Jan 19, 2023
### Rationale for this change

Add Err() to the RecordReader interface so we can report errors.

### Are these changes tested?

This is tested in the C Data Interface.

### Are there any user-facing changes?

**This is a breaking change.**
* Closes: #33789

Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Includes a breaking change to the API Component: Go Type: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants