⚠️ GitHub issue #20213 has been automatically assigned in GitHub to PR creator.

This still needs work, but I would greatly appreciate someone who is more familiar with the arrow code-base to comment on whether my approach is sane or not.

⚠️ GitHub issue #20213 has been automatically assigned in GitHub to PR creator.

Thanks for posting this. The approach is sane, I posted some comments.

You will have to decide whether you want to tackle casting to string, or prefer leaving that for later/another contributor.

So far this PR seems to work for the following casts:

• float16 to float32
• float16 to float64
• float32 to float16
• float64 to float16

@pitrou you mentioned string cast(s).

Is that:

1. float16 to string
2. string to float16

?

What other casts are required to consider this done?

@pitrou you mentioned string cast(s).
Is that:
1. float16 to string
2. string to float16
?

Yes.

What other casts are required to consider this done?

Ideally, integer-to-half and half-to-integer would be implemented as well.

⚠️ GitHub issue #20213 has been automatically assigned in GitHub to PR creator.

Please correct me if I'm wrong, but it seems like float32/float64 cast to string is ultimately handled by this vendored library cpp/src/arrow/vendored/double-conversion, source repo here: https://github.com/google/double-conversion .

There doesn't seem to be a direct way to cast from a half float ultimately represented by a uint16_t to a string, but I wonder if it's acceptable to cast to float first and then use the existing float to string cast? or is that unacceptable overhead?

There doesn't seem to be a direct way to cast from a half float ultimately represented by a uint16_t to a string, but I wonder if it's acceptable to cast to float first and then use the existing float to string cast? or is that unacceptable overhead?

Yes, it's acceptable. The only annoyance would be the superfluous precision, but that's not a blocking issue.

By the way, you'll have to implement unit tests at some point (in C++).

@ClifHouck thanks for working on this!

Small bit of housekeeping:

There is a note in docs/source/status.rst that says:

(1) Casting to/from Float16 in C++ is not supported.

Could you please push a commit here that removes that note? Looks like you will need to rebase first because there have been some changes in status.rst since you began work on this PR.

So one question I currently have is, should I specialize Datum on HalfFloatType (or similar)? Right now tests are failing when trying to compare expected to actual output from doing casts from, say, int to float16.

So one question I currently have is, should I specialize Datum on HalfFloatType (or similar)?

Which method would you specialize?

So one question I currently have is, should I specialize Datum on HalfFloatType (or similar)?

Which method would you specialize?

After reading more code, more likely I need to specialize or modify CompareFloating, VisitFloatingEquality, etc. But I'm not certain.

After reading more code, more likely I need to specialize or modify CompareFloating, VisitFloatingEquality, etc. But I'm not certain.

If you're struggling on this, someone else can take a look and help. Perhaps @benibus or @felipecrv ?
(or I can get to it later when I have time)

I'm currently stuck on getting a new FloatingToFloating test to pass. AFAICT the other existing tests now pass with the new float16 casts. Trying to make progress on getting this last test to pass today.

⚠️ GitHub issue #20213 has been automatically assigned in GitHub to PR creator.

I think I've fixed things with my latest commit. Turned out a specialization of CastPrimitive was not being called as I expected, and instead the generic template version was being used.

I added a few more TODOs but I think this is very close to being ready for review.

⚠️ GitHub issue #20213 has been automatically assigned in GitHub to PR creator.

Could you rebase on main? The failure was fixed by #40794.

Done.

Any objections to @kou merging this?

Oh, sorry. I missed this. I'll merge this.

Oops, sorry that I couldn't give this a review earlier.

I'll address any additional code-related changes needed here in a follow-up PR.

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 72d20ad.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.