GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice#49132
GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice#49132pitrou merged 5 commits intoapache:mainfrom
Conversation
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for tackling this @abhishek593 . Here are a couple comments to improve this PR.
| TYPED_TEST_SUITE(TestBuilderAppendArraySliceBinary, BinaryAppendArraySliceTypes); | ||
| TYPED_TEST(TestBuilderAppendArraySliceBinary, Binary) { | ||
| this->CheckAppendArraySlice(TypeTraits<TypeParam>::type_singleton()); | ||
| } |
There was a problem hiding this comment.
We don't actually need templating, we can just generate the types dynamically using PrimitiveTypes(), BaseBinaryTypes() etc. This will reduce code generation and compile times slightly.
There was a problem hiding this comment.
Besides, we should also test binary_view and string_view types.
There was a problem hiding this comment.
Done. BaseBinaryTypes, binary_view, string_view, etc. is already present in PrimitiveTypes.
There was a problem hiding this comment.
Thanks. Is there a particular reason for keeping the other test functions separate?
Organisation-wise, we should either have one single test function with all types, or have more granular test functions for the "dynamic" types as well (one for primitives, one for temporals etc.).
I'll leave it to your preference :-)
There was a problem hiding this comment.
No, particular reason. I simply incorporated the feedback given previously. I agree with better organisation of tests, so I have grouped the tests better now.
github-actions
bot
added
awaiting committer review
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
+1, thanks for the updates @abhishek593
|
@github-actions crossbow submit -g cpp |
|
Revision: 06f00c7 Submitted crossbow builds: ursacomputing/crossbow @ actions-c25aa1acd5
|
|
CI failure is unrelated, I'll merge. |
|
After merging your PR, Conbench analyzed the 1 benchmarking run that has been run so far on merge-commit 6080027. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
2 participants
Rationale for this change
Introduce systematic test coverage for
arrow::ArrayBuilder::AppendArraySliceacross all major Arrow data types to ensure correctness for diverse physical layouts.What changes are included in this PR?
Are these changes tested?
Yes, all 30 new cases passed locally with varying null probabilities.
Are there any user-facing changes?
No.