man-group / sparrow

C++20 idiomatic APIs for the Apache Arrow Columnar Format
Apache License 2.0
23 stars 9 forks source link

Incorrect assumption that fixed floating types will always be aliases of `double` and `float` leads to errors #162

Open Klaim opened 2 months ago

Klaim commented 2 months ago

Reported through build2's ci (using my wip packaging) :

In the configurations we build and test on github, float32_t and float64_t usually are aliases to double and float. However in some other configurations we didnt test, these names are not aliases, they are concrete separate types (note also the possibility coming from using our float16_t implementation depending on if the standard library available provides or not fixed floating point types). In our tests, we use double directly in some places where we end up with arrow_traits<double>. When float64_t is an alias to double (like on our ci), arrow_traits<double> and arrow_traits<float64_t> are the same type. When it's not (some configurations on build2's CI) these are different types. At the moment we only define arrow_traits<float64_t>, therefore arrow_traits<double> is never found at compile-time and support for double is therefore non-existent in these configurations, leading to compilation errors when trying to use double in sparrow.

Attempt to fix that issue by adding arrow_traits<double> (and float) leads to duplicate type definition errors in configurations where float64_t are aliases to double, so it's the reverse problem.

In my experiments, the best way to fix this seems to be to have only one arrow_traits<std::floating_point> template which handles all the possible floating point types standard at least. In that case, type_id needs to be set to a value depending on the size of the type. My current experiment half-works (it reveals other similar issues) and I intend to complete it to completely solve the issue.

Note that the same solution can be set for integers, I'll try to see if that works too. That would prevent hitting similar issues on exotic platforms with integers and actually reduce the code size for arrow traits provided by default.

Klaim commented 2 months ago

Fixing the traits by generalizing them (#163) proved to not be enough to completely fix the issue: https://ci.stage.build2.org/@31e628c7-67d7-4179-8e14-807e2f196006

Essentially the issue is that the code in array_common.hpp is limited to work only for the types listed in all_base_types_t, which doesnt contain explicitly the fundamental types (like double), so the code in that header (leading to typed_array construction and arrow_variant) will fail to compile on the platforms where double is a different type than the fixed size floating-point types (and similar for other types potentially having the issue).

I tried several workarounds, including adding the fundamental types to all_base_types_t and some other ideas based on the idea that we will not change the arrow_variant soon, but so far nothing really works so far.

Klaim commented 1 month ago

This was automatically closed but the issue is not completely resolved yet, only partially.