raulcd commented on code in PR #46161:
URL: https://github.com/apache/arrow/pull/46161#discussion_r2055696256
##########
cpp/src/arrow/compute/kernels/test_util.h:
##########
@@ -0,0 +1,30 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
I've initially moved it to `arrow/compute/kernels/test_util_internal.h`, in
that case we require to also add to `libarrow_testing.so` ->
`arrow/compute/test_util_internal.cc` otherwise we get some missing symbols
(`undefined reference to arrow::compute::ValidateOutput(arrow::Datum const&)`):
```
list(APPEND ARROW_TESTING_SRCS compute/kernels/test_util_internal.cc
compute/test_util_internal.cc)
```
I don't think it's great that we are exporting symbols from
`test_util_internal.cc` and that we are adding some of the internals to
`libarrow_testing.so`. See the initial commit:
https://github.com/apache/arrow/pull/46161/commits/6ee47f2c648566cdb6cc1c3527eafc036448c201
The above fails on Windows with `'arrow::compute::RunEndEncodeTableColumns':
inconsistent dll linkage`. I could continue with that route but I would
probably have to remove our current `arrow_compute_kernels_testing` target to
just put the internal utilities on `libarrow_testing.so`.
As I think this goes against a better hygiene of `libarrow_testing.so`, at
this point I have decided to move the utility function to
`arrow/acero/test_util_internal.h` and not require `ARROW_TESTING_EXPORT` at
all. I think the utility function is quite niche and can live there and acero
will always require compute.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
