Repository navigation
Conversation
|
|
…pe to GenerateNumeric<>
…fFloatType to GenerateNumeric<>" This reverts commit 436071f1579afbbfd5d0a4839b87e1b33050b81f.
19c70b1 to
7fe3942
Compare
| } | ||
|
|
||
| // XXX: float16 is not part of NumericTypes() yet | ||
| auto& AllNumericTypes() { |
There was a problem hiding this comment.
Can we make this const auto&, to avoid potentially mutating it from a callee?
| return Generator<Type0, DoubleType, Args...>::Exec; | ||
| case Type::HALF_FLOAT: | ||
| // NOTE: Type::HALF_FLOAT used to not be part of the list of numeric types, | ||
| // so users of this template might start failing to compiler after Arrow 17.x. |
There was a problem hiding this comment.
These are internal helpers, so if Arrow still compiles, I don't think the comment is warranted.
There was a problem hiding this comment.
Cool. It took me a while to realize these were internal. I will remove the comments.
| } | ||
| { | ||
| // XXX: float16() is not in NumericTypes() yet | ||
| auto physical_type = GetPhysicalType(float16()); |
There was a problem hiding this comment.
What is the underlying physical type btw? We should be careful that float16 NaNs sort as described as https://arrow.apache.org/docs/cpp/compute.html#sorts-and-partitions, and we should test for that.
There was a problem hiding this comment.
It's float16 itself. I'm changing the code to make that more obvious.
| DCHECK_OK(func->AddKernel(base)); | ||
| } | ||
| { | ||
| // XXX: float16() is not in PrimitiveTypes() |
There was a problem hiding this comment.
We should eventually deal with this somehow. I think we could add a less stable AllPrimitiveTypes() that includes FLOAT16 and eventually mark PrimitiveTypes() as deprecated.
|
@felipecrv Do we have tests for sorting float16 values and NaNs already? Otherwise, we should add some. Also, is only |
|
@felipecrv Is this ready for review again? |
No. Adding these tests you requested creates a cascade of requirements regarding half-float support that I haven't been able to fix even after putting a lot of effort into it. |
|
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer. |
github-actions
Bot
added
the
Status: stale-warning
Rationale for this change
To simplify loops and unit tests involving numeric types. Special-casing float16 adds complexity.
What changes are included in this PR?
Are these changes tested?
By existing tests that are now extended to run with
float16as input.