[C++] xxhash not installed into dist/lib/include when building C++

[asfimport]

My C++ build setup doesn’t install dist/include/arrow/vendored/xxhash/ but only dist/include/arrow/vendored/xxhash.h. The last time the module was installed was in november 2021.

As arrow/python/arrow_to_pandas.cc includes arrow/util/hashing.h ->  arrow/vendored/xxhash.h  -> arrow/vendored/xxhash/xxhash.h this module is needed to try to build Python C++ API separately from C++ (ARROW-16340).

Reporter: Alenka Frim / @AlenkaF
Assignee: Alenka Frim / @AlenkaF

PRs and other links:

• GitHub Pull Request #13282

_{Note: This issue was originally created as ARROW-16609. Please see the migration documentation for further details.}

[asfimport]

Alenka Frim / @AlenkaF:
Also https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/int_util_internal.h would be needed in dist/include for arrow/python/python_to_arrow.cc:44 and arrow/vendored/portable-snippets for arrow/util/int_util_internal.h:30.

[asfimport]

Weston Pace / @westonpace:
arrow/util/hashing.h (which should probably be named arrow/util/hashing_internal.h) is an internal header file. In addition, int_util_internal.h is an internal header file. These header files are intentionally not installed and they should not be imported outside of their components.

So I think we have a few potential paths here:

• We could decide that we really do want these things to be part of our public API

• We could clone these dependencies into pyarrow so that pyarrow also has to keep a copy of these vendored files

• We could wrap the vendored capabilities in some kind of Arrow API or find some way to mask them

  Could you maybe pinpoint where these internal header files are used in python_to_arrow.cc

[asfimport]

Alenka Frim / @AlenkaF:
Thank you for the comment Weston!

Oh, I see. To make Python build separately for https://issues.apache.org/jira/browse/ARROW-16340 the second option would fit well, that is we could clone the dependencies like we do with cpp/cmake_modules.

These header files are used in python_to_arrow.cc: 

arrow/cpp/src/arrow/python/python_to_arrow.cc

Line 44 in 1cdedc4

#include "arrow/util/int_util_internal.h"

and in arrow_to_pandas.cc:

arrow/cpp/src/arrow/python/arrow_to_pandas.cc

Line 41 in c5a175d

#include "arrow/util/hashing.h"

 

[asfimport]

Alenka Frim / @AlenkaF:
Sorry for not presenting a better usage of the internal header files. Digging into the files I see that:

• python_to_arrow.cc uses next members of arrow::internal (from {}int_util_internal.h{});

  • MultiplyWithOverflow
  • AddWithOverflow
  • SubtractWithOverflow

• arrow_to_pandas.cc uses next member of arrow::internal (from {}hashing.h{}):

  • ScalarMemoTable

[asfimport]

Weston Pace / @westonpace:
I think @pitrou's output would be valuable here. I believe he is out until next week however.

My two cents, fwiw, is:

MultiplyWithOverflow/AddWithOverflow/SubtractWithOverflow - These are shallow wrappers around a vendored version of https://github.com/nemequ/portable-snippets/tree/master/safe-math . I do not think it makes sense to expose these. Instead I think it would make sense for pyarrow to vendor this library on its own (or to find some spot to stick the vendored library in the repo so that both libs can use it).

ScalarMemoTable - This is a more complex utility that would not be very easy to replicate. I think we should consider refactoring arrow::internal::ScalarMemoTable into a public header file (and probably renaming to arrow::ScalarMemoTable).

[asfimport]

Antoine Pitrou / @pitrou:
I think that pragmatically it's ok to start installing xxhash and the safe-math snippets.

Also, we can rename int_util_internal.h to int_util_overflow.h or something similar, to also install it.

[asfimport]

Kouhei Sutou / @kou:
Issue resolved by pull request 13282
#13282