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

Several Builder::append methods returning results even though they are infallible #2071

Closed
jhorstmann opened this issue Jul 14, 2022 · 2 comments · Fixed by #2103
Closed
Labels
good first issue Good for newcomers help wanted question Further information is requested

Comments

@jhorstmann
Copy link
Contributor

Which part is this question about

I noticed that a lot of the append or append_null methods in builders are always returning Ok(()). There are only very few builders that can legitimately return an error, for example dictionaries on overflow or some validation for decimals.

Describe your question

I'm wondering whether this is a design decision, to make all builder apis seem more consistent or if it is maybe a leftover from a time when we had fallible allocation logic.

Additional context

Although the compiler should be able to optimize these away, there could still be a performance impact in some cases. It also makes it more difficult to use these builders in an infallible context.

@jhorstmann jhorstmann added the question Further information is requested label Jul 14, 2022
@HaoYang670
Copy link
Contributor

I had the same concern when updating the binary and list builders

@tustvold
Copy link
Contributor

tustvold commented Jul 16, 2022

I would support changing this. Could possibly be combined with #2054

@alamb alamb changed the title Several Builder::append methods returning results even though they are infallible Several Builder::append methods returning results even though they are infallible Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants