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

feat: add try_field() to Schema #5616

Closed
wants to merge 2 commits into from
Closed

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented Apr 9, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

Schema::field() would panic when the index is out of bounds. This patch adds a safer method try_field() that would perform the bound check.

What changes are included in this PR?

A new method try_field() on Schema

Are there any user-facing changes?

Signed-off-by: Ruihang Xia <[email protected]>
@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 9, 2024
Signed-off-by: Ruihang Xia <[email protected]>
@tustvold
Copy link
Contributor

tustvold commented Apr 9, 2024

I'm honestly not sure about the need for this, given schema.try_field(i) is not appreciably less verbose than schema.fields().get(i) which also has the advantage of returning FieldRef

@waynexia
Copy link
Member Author

waynexia commented Apr 9, 2024

I'm honestly not sure about the need for this, given schema.try_field(i) is not appreciably less verbose than schema.fields().get(i) which also has the advantage of returning FieldRef

I thought the current field() was kind of an "unsafe" function, and maybe field_unchecked() is more suitable. It's easy to find and call this method. But panicking here would be hard to debug as the panic info is just an out-of-bounds. Given it's a long-exist public API I add the new try_field() with some hint in field() so we don't break the public interface. I'd like to propose deprecating and removing/marking it unsafe field() as the next step. As you said users can always schema.fields()[i], but we will not provide an easy-to-called unsafe API.

@tustvold
Copy link
Contributor

tustvold commented Apr 9, 2024

I thought the current field() was kind of an "unsafe" function

I think I would prefer to keep things as they are, I don't see an issue with panicking when an invariant is violated, especially as you get a helpful backtrace to the point of issue. I personally don't subscribe to the notion that panics are some evil thing to be avoided, they're just exceptions and they avoid literring code with unnecessary error handling verbosity.

Given there are simple alternatives for people who want to avoid panics for whatever ideological reasons, I think we're ok?

@tustvold tustvold closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants