Skip to content

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

Open
thisisnic wants to merge 2 commits into
apache:mainfrom
thisisnic:GH-50378-parquet-float16
Open

GH-50378: [R] Reading a parquet with a Float16 column yields incorrect value#50451
thisisnic wants to merge 2 commits into
apache:mainfrom
thisisnic:GH-50378-parquet-float16

Conversation

@thisisnic

@thisisnic thisisnic commented Jul 9, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Wrong values reading Parquet files with float16s in them as we were returning the raw bytes not the float value

What changes are included in this PR?

Return the float value. Also enabling float16 in a lot of our tests.

Are these changes tested?

Aye

Are there any user-facing changes?

Nah

Copilot AI review requested due to automatic review settings July 9, 2026 15:32
@thisisnic thisisnic requested a review from jonkeane as a code owner July 9, 2026 15:32
@github-actions github-actions Bot added Component: R awaiting committer review Awaiting committer review labels Jul 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes incorrect decoding of Parquet Float16 values in the R bindings by ensuring half-float buffers are interpreted as IEEE-754 float16 values (instead of being exposed as raw uint16 bit patterns). This aligns R’s Parquet reading behavior with expected semantics and expands float16() coverage in existing R test suites.

Changes:

  • Decode HalfFloatType values via arrow::util::Float16::FromBits(...).ToDouble() when converting Arrow arrays to R numeric vectors.
  • Implement conversion to HalfFloatType when converting R values into Arrow arrays by producing float16 bit patterns.
  • Enable float16() in additional R test type matrices (chunked arrays and dplyr cast type checks).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
r/tests/testthat/test-dplyr-funcs-type.R Expands cast type coverage to include float16() in the explicit conversion test matrix.
r/tests/testthat/test-chunked-array.R Expands chunked array numeric type coverage to include float16().
r/src/r_to_arrow.cpp Implements conversion to Arrow half-float by generating float16 bit patterns.
r/src/array_to_vector.cpp Fixes half-float decoding when ingesting Arrow arrays into R double vectors.

Comment on lines 32 to 35
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())

@jonkeane jonkeane left a comment

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.

Thanks for jumping on this! A few comments: I think the bot might be right that we still need to add more coverage (or maybe I just didn't see where we have it already??). I also am slightly worried our error for things that aren't implemented didn't pop up when the person tried this. Any thoughts what happened there?

Comment thread r/src/r_to_arrow.cpp
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");

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 😬

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".

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

expect_equal(z$length(), 5L)
expect_equal(z$as_vector(), c(9:10, 1:3))

expect_chunked_roundtrip(list(c(1, 2, 3), c(4, 5, 6)), float64())

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.

Do we need a line like this to actually test the roundtripping?

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants