-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
GH-35636: [C++] Extract two expensive test suites from compute-vector-test #36401
Conversation
@github-actions crossbow submit -g cpp |
add_arrow_compute_test(vector_selection_test | ||
SOURCES | ||
vector_selection_test.cc | ||
test_util.cc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we might want to create an object library for test_util.cc
instead of recompiling it for each target. Can be left as a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tempted to do it, but I was afraid it would make things worse because test_util.cc
is so small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's possible. We can revisit later :-)
Revision: 021fb90 Submitted crossbow builds: ursacomputing/crossbow @ actions-ccd2ce168e |
I think you need to run cmake-format for the lint step to pass... or apply the following patch: diff --git a/cpp/src/arrow/compute/kernels/CMakeLists.txt b/cpp/src/arrow/compute/kernels/CMakeLists.txt
index a84b36a52..9084f279a 100644
--- a/cpp/src/arrow/compute/kernels/CMakeLists.txt
+++ b/cpp/src/arrow/compute/kernels/CMakeLists.txt
@@ -76,14 +76,9 @@ add_arrow_compute_test(vector_test
select_k_test.cc
test_util.cc)
-add_arrow_compute_test(vector_sort_test
- SOURCES
- vector_sort_test.cc
- test_util.cc)
+add_arrow_compute_test(vector_sort_test SOURCES vector_sort_test.cc test_util.cc)
-add_arrow_compute_test(vector_selection_test
- SOURCES
- vector_selection_test.cc
+add_arrow_compute_test(vector_selection_test SOURCES vector_selection_test.cc
test_util.cc)
add_arrow_benchmark(vector_hash_benchmark PREFIX "arrow-compute") |
Nice timings here too:
|
I ran the CMake linter locally and was so confused: it only says the checks doesn't pass. |
The CI failures seem unrelated, I'll merge. Thanks @felipecrv ! |
Conbench analyzed the 6 benchmark runs on commit There were 9 benchmark results indicating a performance regression:
The full Conbench report has more details. |
…vector-test (apache#36401) ### Rationale for this change `arrow-compute-vector-test` is too big and takes a long time to run because of that. ### What changes are included in this PR? Extracting two tests. Timings on my machine (Debug builds with ASAN). ``` debug/arrow-compute-vector-test > /dev/null 11.54s user 0.47s system 99% cpu 12.023 total debug/arrow-compute-vector-sort-test > /dev/null 13.30s user 0.26s system 99% cpu 13.579 total debug/arrow-compute-vector-selection-test > /dev/null 6.97s user 0.22s system 99% cpu 7.207 total ``` ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#35636 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
arrow-compute-vector-test
is too big and takes a long time to run because of that.What changes are included in this PR?
Extracting two tests.
Timings on my machine (Debug builds with ASAN).
Are these changes tested?
Yes.
Are there any user-facing changes?
No.