-
Notifications
You must be signed in to change notification settings - Fork 38
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
test: test with the HalfFloatType
from arrow
#503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I think the repetition is OK...we have a lot of repetition in our tests and while it would be good to go through and reduce that where possible, I prefer getting the test coverage there first.
There is also a few places this could be enabled in the integration tests if you have the bandwidth (no pressure!). (This can also be another PR if you prefer)
arrow-nanoarrow/src/nanoarrow/nanoarrow_testing.hpp
Lines 807 to 819 in 5dc4aee
case NANOARROW_TYPE_FLOAT: | |
case NANOARROW_TYPE_DOUBLE: { | |
// JSON number to float_precision_ decimal places | |
LocalizedStream local_stream_opt(out); | |
local_stream_opt.SetFixed(float_precision_); | |
WriteFloatMaybeNull(out, value, 0); | |
for (int64_t i = 1; i < value->length; i++) { | |
out << ", "; | |
WriteFloatMaybeNull(out, value, i); | |
} | |
break; | |
} |
arrow-nanoarrow/src/nanoarrow/nanoarrow_testing.hpp
Lines 807 to 819 in 5dc4aee
case NANOARROW_TYPE_FLOAT: | |
case NANOARROW_TYPE_DOUBLE: { | |
// JSON number to float_precision_ decimal places | |
LocalizedStream local_stream_opt(out); | |
local_stream_opt.SetFixed(float_precision_); | |
WriteFloatMaybeNull(out, value, 0); | |
for (int64_t i = 1; i < value->length; i++) { | |
out << ", "; | |
WriteFloatMaybeNull(out, value, i); | |
} | |
break; | |
} |
arrow-nanoarrow/src/nanoarrow/nanoarrow_testing.hpp
Lines 2149 to 2152 in 5dc4aee
case NANOARROW_TYPE_FLOAT: | |
return SetBufferFloatingPoint<float>(data, buffer, error); | |
case NANOARROW_TYPE_DOUBLE: | |
return SetBufferFloatingPoint<double>(data, buffer, error); |
Not a problem, I added a helper function and it should look better now. :)
It's done now ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Tests for #507 and #501 and/or #503 either used C++17 or features from Arrow C++ > 12. Our test suite still supports these (although perhaps parts of this support should be dropped soon). On Windows, formatting with `%lu` was doing some unexpected formatting. We could do a better job formatting 64-bit integers in error messages (e.g., using `PRId64` and the requisite defines to ensure it works on mingw); however, we probably won't ever be able to support properly formatting an unsigned 64-bit integer on every platform we support. I changed the error message (and its test) slightly to reflect that.
Hi this PR adds an extra test case that tests
ArrowFloatToHalfFloat
andArrowArrayViewGet{Double,Int,UInt}Unsafe
with theHalfFloatType
from arrow.https://github.com/apache/arrow/blob/255dbf990c3d3e5fb1270a2a11efe0af2be195ab/cpp/src/arrow/type.h#L704-L713
And sorry that I didn't know there's a
HalfFloatType
in arrow-cpp when I was doing #501 🥲.Sadly that we cannot simply appendTestGetFromNumericArrayView<HalfFloatType>();
at the end of theArrayViewTestGetNumeric
test suite because we have to convert floats to half-floats usingArrowFloatToHalfFloat
before callingbuilder.Append
(otherwise we'll get weird values back in the subsequentArrowArrayViewGet{Double,Int,UInt}Unsafe
calls).Added some simple C++ magic and we can simply append
TestGetFromNumericArrayView<HalfFloatType>();
at the end of theArrayViewTestGetNumeric
test suite now.