kou commented on code in PR #46922:
URL: https://github.com/apache/arrow/pull/46922#discussion_r2173521447
##########
cpp/src/arrow/compute/CMakeLists.txt:
##########
@@ -114,6 +114,46 @@ function(ADD_ARROW_COMPUTE_TEST REL_TEST_NAME)
${ARG_UNPARSED_ARGUMENTS})
endfunction()
+# This function is used to add a custom main to the benchmarks in order
+# to initialize the compute kernels registry before running them.
+# This is necessary for benchmarks that use compute kernels that are not
+# part of libarrow.
+# It will also link the compute libraries to the benchmark target.
+function(ADD_ARROW_COMPUTE_BENCHMARK REL_TEST_NAME)
Review Comment:
Could you use lower case for newly added function names?
```suggestion
function(add_arrow_compute_benchmark REL_TEST_NAME)
```
(And I want to use `arrow_` prefix for our CMake functions/macros...)
##########
cpp/src/arrow/compute/CMakeLists.txt:
##########
@@ -142,7 +182,9 @@ add_arrow_compute_test(row_test
EXTRA_LINK_LIBS
arrow_compute_testing)
-add_arrow_benchmark(function_benchmark PREFIX "arrow-compute")
+if(ARROW_BUILD_BENCHMARKS)
Review Comment:
Can we remove this `if(ARROW_BUILD_BENCHMARKS)`?
We want to do it only in `add_benchmark()`.
##########
cpp/src/arrow/compute/CMakeLists.txt:
##########
@@ -114,6 +114,46 @@ function(ADD_ARROW_COMPUTE_TEST REL_TEST_NAME)
${ARG_UNPARSED_ARGUMENTS})
endfunction()
+# This function is used to add a custom main to the benchmarks in order
+# to initialize the compute kernels registry before running them.
+# This is necessary for benchmarks that use compute kernels that are not
+# part of libarrow.
+# It will also link the compute libraries to the benchmark target.
+function(ADD_ARROW_COMPUTE_BENCHMARK REL_TEST_NAME)
+ set(options)
+ set(one_value_args PREFIX)
+ set(multi_value_args EXTRA_SOURCES)
+ cmake_parse_arguments(ARG
+ "${options}"
+ "${one_value_args}"
+ "${multi_value_args}"
+ ${ARGN})
+ if(ARG_PREFIX)
+ set(PREFIX ${ARG_PREFIX})
+ else()
+ set(PREFIX "arrow-compute")
+ endif()
+ set(EXTRA_SOURCES
"${CMAKE_CURRENT_FUNCTION_LIST_DIR}/main_compute_benchmark.cc")
+ if(ARG_EXTRA_SOURCES)
+ list(APPEND EXTRA_SOURCES ${ARG_EXTRA_SOURCES})
+ endif()
+ add_benchmark(${REL_TEST_NAME}
+ PREFIX
+ ${PREFIX}
+ LABELS
+ "arrow-benchmarks"
+ EXTRA_SOURCES
+ ${EXTRA_SOURCES}
+ OUTPUT_BENCHMARK_NAME
+ COMPUTE_BENCHMARK_NAME
+ ${ARG_UNPARSED_ARGUMENTS})
+ if(ARROW_BUILD_STATIC)
+ target_link_libraries(${COMPUTE_BENCHMARK_NAME} PUBLIC
arrow_compute_static)
+ else()
+ target_link_libraries(${COMPUTE_BENCHMARK_NAME} PUBLIC
arrow_compute_shared)
+ endif()
Review Comment:
How about adding `EXTRA_PUBLIC_LINK_LIBS` to `add_benchmark()`?
(Do we need to use `PUBLIC` here? Can we use `PRIVATE`?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
