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.

Sign up for GitHub

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

Jump to bottom

[Go] C Data Interface should import schema up front instead of panicking #35974

Closed
lidavidm opened this issue Jun 7, 2023 · 0 comments · Fixed by #35978
Closed

[Go] C Data Interface should import schema up front instead of panicking #35974

lidavidm opened this issue Jun 7, 2023 · 0 comments · Fixed by #35978
Assignees
@lidavidm
Labels
Component: Go Type: enhancement
Milestone
13.0.0

Comments

@lidavidm
Copy link
Member

lidavidm commented Jun 7, 2023

Describe the enhancement requested

Same issue that bit the C++ implementation: Schema() is infallible so it can't be implemented on top of C Data Interface (where get_schema is fallible) without panicking. We should import the schema up front and report the error properly.

Component(s)

Go
@lidavidm lidavidm self-assigned this Jun 7, 2023
@github-actions github-actions bot mentioned this issue Jun 7, 2023
Merged
lidavidm added a commit to lidavidm/arrow that referenced this issue Jun 7, 2023
@lidavidm
apacheGH-35974: [Go] Don't panic if importing C Array Stream fails
faee715
lidavidm added a commit to lidavidm/arrow that referenced this issue Jun 7, 2023
@lidavidm
apacheGH-35974: [Go] Don't panic if importing C Array Stream fails
fb5256b
lidavidm added a commit to lidavidm/arrow that referenced this issue Jun 7, 2023
@lidavidm
apacheGH-35974: [Go] Don't panic if importing C Array Stream fails
1996436
lidavidm added a commit to lidavidm/arrow that referenced this issue Jun 7, 2023
@lidavidm
apacheGH-35974: [Go] Don't panic if importing C Array Stream fails
cfd90d4
lidavidm added a commit that referenced this issue Jun 7, 2023
@lidavidm
GH-35974: [Go] Don't panic if importing C Array Stream fails (#35978)
16328f0 
### Rationale for this change
Panicking is rude.

### What changes are included in this PR?

Import C Array Stream schemas up front and report the error.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

`ImportCArrayStream` (which cannot fail, only panic) is deprecated in favor of `ImportCRecordReader` (which can return an error).
* Closes: #35974

Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@lidavidm lidavidm added this to the 13.0.0 milestone Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lidavidm lidavidm

Labels
Component: Go Type: enhancement
Projects
None yet
Milestone
13.0.0
Development

Successfully merging a pull request may close this issue.

GH-35974: [Go] Don't panic if importing C Array Stream fails lidavidm/arrow
1 participant
@lidavidm