Skip to content

GH-50378: [R] Reading a parquet with a Float16 column yields incorrect value #50451

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.

Sign up for GitHub

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

Open
thisisnic wants to merge 2 commits into
base: main
Choose a base branch
from
+11 −5
Open

GH-50378: [R] Reading a parquet with a Float16 column yields incorrect value #50451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bec672
Add float16 logic
thisisnic Jul 9, 2026
defb6d1
add float16 into tests
thisisnic Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion r/src/array_to_vector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <arrow/table.h>
#include <arrow/util/bitmap_reader.h>
#include <arrow/util/bitmap_writer.h>
#include <arrow/util/float16.h>
#include <arrow/util/int_util.h>

#include <type_traits>
Expand Down Expand Up @@ -224,7 +225,11 @@ class Converter_Double : public Converter {
}
auto p_data = REAL(data) + start;
auto ingest_one = [&](R_xlen_t i) {
p_data[i] = static_cast<value_type>(p_values[i]);
if constexpr (std::is_same_v<Type, HalfFloatType>) {
p_data[i] = arrow::util::Float16::FromBits(p_values[i]).ToDouble();
} else {
p_data[i] = static_cast<value_type>(p_values[i]);
}
return Status::OK();
};
auto null_one = [&](R_xlen_t i) {
Expand Down
5 changes: 3 additions & 2 deletions r/src/r_to_arrow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <arrow/util/bitmap_writer.h>
#include <arrow/util/checked_cast.h>
#include <arrow/util/converter.h>
#include <arrow/util/float16.h>
#include <arrow/util/logging.h>

#include "./r_task_group.h"
Expand Down Expand Up @@ -390,12 +391,12 @@ struct RConvert {
return static_cast<float>(from);
}

// ---- convert to half float: not implemented
// ---- convert to half float
template <typename Type, typename From>
static enable_if_t<std::is_same<Type, const HalfFloatType>::value,
Result<typename Type::c_type>>
Convert(Type*, From from) {
return Status::Invalid("Cannot convert to Half Float");

@jonkeane jonkeane Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we didn't get this error when someone tried this. Any ideas why this didn't bonk? It's not super important for us here since we have now implemented this, but I am worried there might be other things that we have that are supposed to bonk like this but actually aren't 😬

@thisisnic thisisnic Jul 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That bit there is enabling the R to Arrow direction, but the original issue was hitting the Arrow to R direction. My assumption is that we chose not to implement it because it's not really useful for most R users to have it? Like float16s are weird because you can't represent as many numbers, e.g.

> arrow_array(c(1.1, 2.2, 3.3), float16())
Array
<halffloat>
[
  1.099609375,
  2.19921875,
  3.30078125
]

And actually, they don't roundtrip nicely at all

> arrow_array(c(1.1, 2.2, 3.3), float16())$as_vector()
[1] 1.099609 2.199219 3.300781

Though you could say 1.1 was never a float16 in the first place. Maybe I should just ditch the R to Arrow stuff; it felt like making a more complete PR, but we might just be handing users a footgun, as I can image finding it easy to go "this will take up less storage, let's use it".

@pitrou pitrou Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.1 was never a float64 either :)

>>> 1.1 - 1
0.10000000000000009

return arrow::util::Float16(static_cast<double>(from)).bits();
}
};

Expand Down
2 changes: 1 addition & 1 deletion r/tests/testthat/test-chunked-array.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

int_types <- c(int8(), int16(), int32(), int64())
uint_types <- c(uint8(), uint16(), uint32(), uint64())
float_types <- c(float32(), float64()) # float16() not really supported in C++ yet
float_types <- c(float16(), float32(), float64())
all_numeric_types <- c(int_types, uint_types, float_types)

expect_chunked_roundtrip <- function(x, type) {
Expand Down
2 changes: 1 addition & 1 deletion r/tests/testthat/test-dplyr-funcs-type.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ test_that("explicit type conversions with cast()", {

int_types <- c(int8(), int16(), int32(), int64())
uint_types <- c(uint8(), uint16(), uint32(), uint64())
float_types <- c(float32(), float64())
float_types <- c(float16(), float32(), float64())

Comment on lines 32 to 35
types <- c(
int_types,
Expand Down
Loading