GH-45723: [C++] FixedSizeListBuilder should have UnsafeAppend methods#46126
GH-45723: [C++] FixedSizeListBuilder should have UnsafeAppend methods#46126cramosme wants to merge 6 commits intoapache:mainfrom
Conversation
Edited scalar_nested.cc and fixed_width_test_util.cc. Ran ctest after and all test still pass
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
cramosme
changed the title
|
|
| /// using the child array builder. | ||
| Status Append(); | ||
|
|
||
| void UnsafeAppend(); |
There was a problem hiding this comment.
- Can you please add docstrings for these new methods?
- Ideally, the definition of the methods should be inline here, so that they can be inlined in the caller.
|
|
||
| void FixedSizeListBuilder::UnsafeAppendNull() { | ||
| UnsafeAppendToBitmap(false); | ||
| (void) value_builder_->AppendNulls(list_size_); |
There was a problem hiding this comment.
We don't want to ignore error returns, so instead we should call a hypothetical UnsafeAppendNulls(list_size_).
If such a method doesn't exist, it should be added.
| auto* fsl_builder = checked_cast<FixedSizeListBuilder*>(builder); | ||
| assert(list_size == checked_cast<FixedSizeListType&>(*type).list_size()); | ||
| RETURN_NOT_OK(fsl_builder->Append()); | ||
| fsl_builder->UnsafeAppend(); |
There was a problem hiding this comment.
I don't think we should change this one. This is a testing utility, so we value robustness over performance.
2 participants
Rationale for this change
What changes are included in this PR? I added the three UnsafeAppend methods for FixedSizeListBuilder and replaced
instances in three files to call UnsafeAppend instead of regular append. Files changed were: scalar_nested.cc, fixed_width_test_util.cc, and scalar_string_ascii.cc, along with the header and cc file where functions were declared/defined.
Are these changes tested? Upon each change, I recompiled and rebuilt the program and tested with ctest. Through my testing, everything seemed to be working as usual.
Are there any user-facing changes?