GH-36036: [C++][Python][Parquet] Implement Float16 logical type by benibus · Pull Request #36073 · apache/arrow

benibus · 2023-06-14T19:39:01Z

Rationale for this change

There is currently an active proposal to support half-float types in Parquet. For more details/discussion, see the links in this PR's accompanying issue.

What changes are included in this PR?

This PR implements basic support for a Float16LogicalType in accordance with the proposed spec. More specifically, this includes:

Changes to parquet.thrift and regenerated parqet_types files
Basic LogicalType class definition, method impls, and enums
Support for specialized comparisons and column statistics

In the interest of scope, this PR does not currently deal with arrow integration and byte split encoding - although we will want both of these features resolved before the proposal is approved.

Are these changes tested?

Yes (tests are included)

Are there any user-facing changes?

Yes

Closes: [C++][Parquet] Implement Float16 logical type #36036

benibus · 2023-06-14T23:40:37Z

cc @anjakefala @zeroshade @pitrou

cpp/src/parquet/float_internal.h

pitrou

Thanks @benibus ! Please note that the Thrift definitions will have to be synchronized from https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift once the Parquet format spec addition is accepted.

cpp/src/parquet/float_internal.h

cpp/src/parquet/statistics.cc

cpp/src/parquet/statistics_test.cc

benibus · 2023-06-15T15:18:49Z

Thanks! Before moving forward on some of the suggestions, I just want to ensure that I interpreted the endianness requirement from the proposal correctly. As per the spec:

The FLOAT16 annotation represents half-precision floating-point numbers in the 2-byte IEEE little-endian format.

Is this in line with what I've done here (i.e. the FLBA itself representing a little-endian binary float) or does it imply something else?

pitrou · 2023-06-15T15:29:01Z

Is this in line with what I've done here (i.e. the FLBA itself representing a little-endian binary float)

Yes!

cpp/src/parquet/float_internal.h

wgtmac · 2023-06-16T09:42:06Z

cpp/src/parquet/types.cc

We may need to change thrift_internal.h for serialization of FLOAT16.

cpp/src/parquet/statistics.cc

cpp/src/arrow/util/float16.h

cpp/src/arrow/util/float16_test.cc

cpp/src/parquet/statistics.cc

pitrou · 2023-06-19T15:57:22Z

By the way:

In the interest of scope, this PR does not currently deal with arrow integration and byte split encoding - although we will want both of these features resolved before the proposal is approved.

Do you plan to tackle Arrow integration in a subsequent PR? This PR cannot be integrated before the proposal is approved, AFAICT.

benibus · 2023-06-19T18:32:07Z

Do you plan to tackle Arrow integration in a subsequent PR? This PR cannot be integrated before the proposal is approved, AFAICT.

Yes, my plan was to handle Arrow integration in a subsequent PR (in tandem with the Java implementation), and then move forward with the proposal.

However, I think you're right that we'll need to roll those features into this PR. Just did a closer re-reading of @julienledem's comments on apache/parquet-format#184, and I believe we're going to want the full C++/Java PRs ready (but not merged) before approving the proposal - so we'll probably need to sit on this one in the meantime.

That being said, I can maintain this PR until then. Plus we may need to make adjustments if new requirements come in from the Java side.

pitrou · 2023-06-21T14:07:26Z

Well, if Arrow integration does not come in this PR, then at least some testing of reading/writing using non-Arrow Parquet APIs should still be added.

wgtmac · 2023-06-21T14:45:50Z

Just curious: Is it enough to get approval if there is a full C++ implementation only? Or java parity should be ready at the meanwhile?

pitrou · 2023-06-21T14:49:05Z

We probably want a Java implementation as well according to apache/parquet-format#184 (comment)

wgtmac · 2023-06-21T16:20:48Z

We probably want a Java implementation as well according to apache/parquet-format#184 (comment)

OK, if that is a must. I can spare some time to do this. Would be good after this PR gets completed.

Reverted several prior changes that were accidentally pushed Enabled construction from native floats Removed `uint16_t` conversion operator since it doesn't behave consistently with standard floats. As a result, rolled back some of the prior changes to `random_real` used in the Parquet test utils

### Rationale for this change There is an active proposal for a Float16 logical type in Parquet (apache/parquet-format#184) with C++/Python implementations in progress (#36073), so we should add one for Go as well. ### What changes are included in this PR? - [x] Adds `LogicalType` definitions and methods for `Float16` - [x] Adds support for `Float16` column statistics and comparators - [x] Adds support for interchange between Parquet and Arrow's half-precision float ### Are these changes tested? Yes ### Are there any user-facing changes? Yes * Closes: #37582 Authored-by: benibus <bpharks@gmx.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

benibus · 2023-11-13T22:00:21Z

Now that the proposal is merged, would anyone be willing to take a final look and potentially merge this? I've gone ahead and rebased for good measure.

I should also mention that the Go implementation was merged today, but it'll eventually need this PR's parquet.thrift to re-generate its files in the future.

…37599) ### Rationale for this change There is an active proposal for a Float16 logical type in Parquet (apache/parquet-format#184) with C++/Python implementations in progress (apache#36073), so we should add one for Go as well. ### What changes are included in this PR? - [x] Adds `LogicalType` definitions and methods for `Float16` - [x] Adds support for `Float16` column statistics and comparators - [x] Adds support for interchange between Parquet and Arrow's half-precision float ### Are these changes tested? Yes ### Are there any user-facing changes? Yes * Closes: apache#37582 Authored-by: benibus <bpharks@gmx.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

cpp/src/arrow/util/float16.h

cpp/src/parquet/arrow/test_util.h

pitrou

LGTM except for the nits above.

Would it be useful to add tests checking that one write or read a Parquet FLOAT16 with a byte length != 2?

mapleFU

LGTM

( It's a bit weird for me that fp16 can only use PLAIN, RLE_DICT and ... DELTA_BYTE_ARRAY, aha...)

benibus · 2023-11-14T17:58:49Z

Would it be useful to add tests checking that one write or read a Parquet FLOAT16 with a byte length != 2?

Probably... I'll try to add something, assuming it wouldn't be redundant.

( It's a bit weird for me that fp16 can only use PLAIN, RLE_DICT and ... DELTA_BYTE_ARRAY, aha...)

Yeah, I think we're going to specifically address the encodings as a follow-up since there's been some recent discussion in that area.

benibus · 2023-11-14T20:48:36Z

Update: I don't think it'd be very useful to add tests for different type lengths at the reader/writer level since it isn't even possible to construct a PrimitiveNode with an inapplicable length without throwing an exception. I suppose that's why there aren't similar test for other (e.g. decimal, uuid) types AFAICT.

However, it turns out that I forgot to add the relevant tests to schema_test.cc, so I went ahead and included those - along with the other requested changes.

pitrou · 2023-11-15T10:14:36Z

LGTM, I'm waiting for a last CI check and will merge afterwards.

mapleFU · 2023-11-15T10:25:59Z

Also we're going to release this in parquet-2.10

apache/parquet-format#219

conbench-apache-arrow · 2023-11-15T14:43:23Z

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

There were no benchmark performance regressions. 🎉

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

…37599) ### Rationale for this change There is an active proposal for a Float16 logical type in Parquet (apache/parquet-format#184) with C++/Python implementations in progress (apache#36073), so we should add one for Go as well. ### What changes are included in this PR? - [x] Adds `LogicalType` definitions and methods for `Float16` - [x] Adds support for `Float16` column statistics and comparators - [x] Adds support for interchange between Parquet and Arrow's half-precision float ### Are these changes tested? Yes ### Are there any user-facing changes? Yes * Closes: apache#37582 Authored-by: benibus <bpharks@gmx.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

…apache#36073) ### Rationale for this change There is currently an active proposal to support half-float types in Parquet. For more details/discussion, see the links in this PR's accompanying issue. ### What changes are included in this PR? This PR implements basic support for a `Float16LogicalType` in accordance with the proposed spec. More specifically, this includes: - Changes to `parquet.thrift` and regenerated `parqet_types` files - Basic `LogicalType` class definition, method impls, and enums - Support for specialized comparisons and column statistics In the interest of scope, this PR does not currently deal with arrow integration and byte split encoding - although we will want both of these features resolved before the proposal is approved. ### Are these changes tested? Yes (tests are included) ### Are there any user-facing changes? Yes * Closes: apache#36036 Lead-authored-by: benibus <bpharks@gmx.com> Co-authored-by: Ben Harkins <60872452+benibus@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Antoine Pitrou <antoine@python.org>

### Rationale for this change There is an active proposal for a Float16 logical type in Parquet (apache/parquet-format#184) with C++/Python implementations in progress (apache/arrow#36073), so we should add one for Go as well. ### What changes are included in this PR? - [x] Adds `LogicalType` definitions and methods for `Float16` - [x] Adds support for `Float16` column statistics and comparators - [x] Adds support for interchange between Parquet and Arrow's half-precision float ### Are these changes tested? Yes ### Are there any user-facing changes? Yes * Closes: #37582 Authored-by: benibus <bpharks@gmx.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

github-actions bot added Component: C++ Component: Parquet awaiting review Awaiting review labels Jun 14, 2023

benibus force-pushed the GH-36036-parquet-float16 branch from 47790ee to 2809fbf Compare June 14, 2023 22:07

benibus marked this pull request as ready for review June 14, 2023 23:39

benibus requested a review from wjones127 as a code owner June 14, 2023 23:39

mapleFU reviewed Jun 15, 2023

View reviewed changes

cpp/src/parquet/float_internal.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 15, 2023

pitrou reviewed Jun 15, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 15, 2023

anjakefala mentioned this pull request Jun 15, 2023

PARQUET-758: Add Float16/Half-float logical type apache/parquet-format#184

Merged

3 tasks

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 15, 2023

wgtmac reviewed Jun 16, 2023

View reviewed changes

benibus force-pushed the GH-36036-parquet-float16 branch 3 times, most recently from 5c109d3 to 2f9ca2a Compare June 18, 2023 03:42

benibus requested a review from pitrou June 18, 2023 04:14

pitrou reviewed Jun 19, 2023

View reviewed changes

benibus added 7 commits November 13, 2023 15:31

Change misleading types

7407ce4

Refactor typed comparators

354f6f6

Add logical type to docs

ea5f5dc

Replace public Float16(uint16_t) constructor

ab84630

Float16 tweaks, add constexpr tests

c8404bb

Add test for ColumnIndex/BoundaryOrder

157e0d7

benibus force-pushed the GH-36036-parquet-float16 branch from a672c73 to 157e0d7 Compare November 13, 2023 21:01

pitrou reviewed Nov 14, 2023

View reviewed changes

cpp/src/arrow/util/float16.h Outdated Show resolved Hide resolved

pitrou reviewed Nov 14, 2023

View reviewed changes

cpp/src/arrow/util/float16.h Outdated Show resolved Hide resolved

pitrou reviewed Nov 14, 2023

View reviewed changes

cpp/src/parquet/arrow/test_util.h Outdated Show resolved Hide resolved

pitrou approved these changes Nov 14, 2023

View reviewed changes

mapleFU approved these changes Nov 14, 2023

View reviewed changes

benibus added 2 commits November 14, 2023 12:36

Tweak ToEndian methods

5eb90d7

Relocate random Float16 function

cb17f56

Add missing schema tests

36b8a3b

pitrou merged commit b55d13c into apache:main Nov 15, 2023

pitrou removed the awaiting merge Awaiting merge label Nov 15, 2023

adamreeve mentioned this pull request Jan 26, 2024

[C++] vendor a half precision floating point library #22806

Closed

Conversation

benibus commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

benibus commented Jun 14, 2023

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benibus commented Jun 15, 2023

Uh oh!

pitrou commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

wgtmac Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Jun 19, 2023

Uh oh!

benibus commented Jun 19, 2023

Uh oh!

pitrou commented Jun 21, 2023

Uh oh!

wgtmac commented Jun 21, 2023

Uh oh!

pitrou commented Jun 21, 2023

Uh oh!

wgtmac commented Jun 21, 2023

Uh oh!

benibus commented Nov 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

benibus commented Nov 14, 2023

Uh oh!

benibus commented Nov 14, 2023

Uh oh!

pitrou commented Nov 15, 2023

Uh oh!

mapleFU commented Nov 15, 2023

Uh oh!

conbench-apache-arrow bot commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

benibus commented Jun 14, 2023 •

edited

Loading

pitrou left a comment •

edited

Loading