Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Klaim opened this issue Aug 3, 2024 · 2 comments · Fixed by #163
Assignees
Labels
bug Something isn't working

Comments

@Klaim
Copy link
Collaborator

Klaim commented Aug 3, 2024

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
Copy link
Collaborator Author

Klaim commented Aug 5, 2024

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 Klaim added the bug Something isn't working label Aug 12, 2024
@Klaim
Copy link
Collaborator Author

Klaim commented Sep 3, 2024

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

@Klaim Klaim reopened this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant