WillAyd commented on code in PR #47282:
URL: https://github.com/apache/arrow/pull/47282#discussion_r2261319860


##########
cpp/src/arrow/compute/kernels/ree_util_internal.h:
##########
@@ -347,7 +347,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateRunEndsArray(
 /// \param has_validity_buffer a validity buffer must be allocated
 /// \param length the length of the values array
 /// \param data_buffer_size the size of the data buffer for string and binary 
types
-ARROW_COMPUTE_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray(
+ARROW_EXPORT Result<std::shared_ptr<ArrayData>> PreallocateValuesArray(

Review Comment:
   Although this file is in the compute/kernels tree, the associated module is 
built as part of the arrow lib irrespective of whether compute is enabled or 
not. As such, I figured it made more sense to use the EXPORT semantics from the 
arrow lib rather than arrow_compute



##########
cpp/src/arrow/util/meson.build:
##########
@@ -224,18 +222,25 @@ utility_test_srcs = [
 ]
 
 if host_machine.system() == 'windows'
+    # meson does not natively support manifest files like CMake does

Review Comment:
   I don't think the Meson support for manifest files is as nice as CMake's, 
but I'm also not super familiar with them. Need to research this a bit more...



##########
cpp/src/arrow/compute/CMakeLists.txt:
##########
@@ -52,6 +52,7 @@ if(ARROW_TESTING AND ARROW_COMPUTE)
     target_link_libraries(arrow_compute_testing
                           PUBLIC $<TARGET_OBJECTS:arrow_compute_core_testing>
                           PUBLIC ${ARROW_GTEST_GTEST_MAIN})
+    target_compile_definitions(arrow_compute_testing PRIVATE COMPILED_BY_CMAKE)

Review Comment:
   Not a huge fan of this, but there appears to be an issue with the registry 
on MSVC with the Meson configuration that isn't in the CMake config.
   
   I think this has to do with the CMake config forcing a certain type of 
linkage, but I'm not sure yet. I am also under the impression that the 
Meson/non-MSVC path is preferred, so we might prefer to "fix" CMake rather than 
make the Meson configuration match it



-- 
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

Reply via email to