GH-45344: [C++][Testing] Generic StepGenerator
#45345
Conversation
|
|
|
Hi @pitrou @westonpace , does this make sense? Thanks. |
| ARROW_TESTING_EXPORT std::shared_ptr<ArrayGenerator> Step(T start = 0, T step = 1) { | ||
| class StepGenerator : public ArrayGenerator { | ||
| public: | ||
| // Use [[maybe_unused]] to avoid a compiler warning in Clang versions before 15 that |
There was a problem hiding this comment.
This is weird but it seems to be a clang bug.
CI failure: https://github.com/apache/arrow/actions/runs/12952919478/job/36131370510?pr=45345
/arrow/cpp/src/arrow/testing/generator.h:313:11: error: unused type alias 'ArrowType' [-Werror,-Wunused-local-typedef]
using ArrowType = typename CTypeTraits<T>::ArrowType;
^
A SO thread: https://stackoverflow.com/questions/50205243/clang-emits-an-unused-type-alias-warning-for-a-type-alias-that-is-used
Reproduction on Clang 14: https://godbolt.org/z/hsnh6cGh8
Clang 15 (works fine): https://godbolt.org/z/x5zTe7j64
github-actions
bot
added
awaiting committer review
github-actions
bot
added
awaiting merge
cpp/src/arrow/testing/generator.h
Outdated
| @@ -23,6 +23,7 @@ | |||
| #include <vector> | |||
|
|
|||
| #include "arrow/array/array_base.h" | |||
| #include "arrow/builder.h" | |||
There was a problem hiding this comment.
This is a very large inclusion, can we avoid it here? Perhaps only include arrow/array/builder_primitive.h?
There was a problem hiding this comment.
Or you can perhaps just rely on TypedBufferBuilder<T> below.
There was a problem hiding this comment.
Thank you. I have changed to arrow/buffer_builder.h (for TypedBufferBuilder) + arrow/array/util.h (for MakeArray). This reduced the preprocessing result from 4.0M to 3.7M.
Co-authored-by: Antoine Pitrou <[email protected]>
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d1bf856. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
#45344
What changes are included in this PR?
Make the
StepGeneratorgeneric.Are these changes tested?
UT included.
Are there any user-facing changes?
None.
StepGeneratorfor test data generation #45344