PARQUET-1647: [Java][Parquet] Implement FLOAT16 logical type

[zhangjiashen]

This is to implement logical type Float16.

Jira

• My PR addresses the following Parquet Jira issue
  • https://issues.apache.org/jira/browse/PARQUET-1647
  • References:
    1. PARQUET-758: Add Float16/Half-float logical type parquet-format#184
    2. PARQUET-1253: Support for new logical type representation #463
    3. GH-36036: [C++][Python][Parquet] Implement Float16 logical type arrow#36073
    4. GH-37582: [Go][Parquet] Implement Float16 logical type arrow#37599
    5. https://github.com/benibus/parquet-mr/tree/PARQUET-1647-float16
    6. https://github.com/anjakefala/parquet-format/tree/halffloat

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:
• Unit Tests
• E2E Tests

Commits

• My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain Javadoc that explain what it does

[benibus]

CI failures are likely due to the fact that the addition of the logical type to parquet-format is unmerged, so the specific PR branch needs to be manually installed for the build to pass. I'm not sure if there's a good solution yet, as this implementation needs to be present for said parquet-format PR to be voted on and merged.

[benibus]

Thanks! This LGTM so far (good work on the conversions), just a few comments.

[wgtmac]

Thanks for working on this @zhangjiashen!

I think there are some follow-up work items:

• Add a Float16Statistics as suggested by @benibus. Internally it leverages the BINARY_AS_FLOAT16_COMPARATOR to collect min/max values. Also make sure it is well covered in the unit test.
• Add some e2e tests to make sure statistics are correctly collected by the parquet writer and can be read from the parquet reader.

[wgtmac]

CI failures are likely due to the fact that the addition of the logical type to parquet-format is unmerged, so the specific PR branch needs to be manually installed for the build to pass. I'm not sure if there's a good solution yet, as this implementation needs to be present for said parquet-format PR to be voted on and merged.

I agree that this is not elegant. For now, we can only review the functionality and completeness of the current implementation. Once the format change is submitted (and released), we need to take another pass on this PR.

[zhangjiashen]

CI failures are likely due to the fact that the addition of the logical type to parquet-format is unmerged, so the specific PR branch needs to be manually installed for the build to pass. I'm not sure if there's a good solution yet, as this implementation needs to be present for said parquet-format PR to be voted on and merged.

Agree! we can merge PR first after this diff is ready

[zhangjiashen]

@wgtmac @benibus please help revisit this PR once you get a chance?

[wgtmac]

@wgtmac @benibus please help revisit this PR once you get a chance?

I will take a look later this week. cc @shangxinli @gszadovszky

[wgtmac]

Thanks for addressing the comments quickly! I have left some comments mainly with regard to the issue that we should only use FIXED_LENGTH_BYTE_ARRAY type not BINARY type.

Other than that, we are missing an E2E test case. Please follow what https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetWriterTruncation.java does to write some float16 values in several row groups and check if the statistics from reader are correct.

[zhangjiashen]

@wgtmac @benibus @gszadovszky Thanks a lot for reviewing this PR and providing good suggestions, most of them are addressed and could you please help take a look again in case I miss any, should we first merge parquet-format PR and release a lib for this PR if we think this PR is ready? CC: @shangxinli

[wgtmac]

I didn't see the e2e test yet, please check https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetWriterTruncation.java for reference.

[wgtmac]

Thanks @zhangjiashen!

+1

[wgtmac]

apache/parquet-format#184 is merged. Could you try to set parquet.format.version to 2.10.0-SNAPSHOT in the pom.xml and check if the CIs are green?

[gszadovszky]

@wgtmac, I don't think we automatically deploy snapshot versions. And, we will need a final release of parquet-format anyway, before we can get this one merged.

[wgtmac]

@wgtmac, I don't think we automatically deploy snapshot versions. And, we will need a final release of parquet-format anyway, before we can get this one merged.

OK, then let's wait until format v2.10 is released. Once two PoC implementations of apache/parquet-format#197 have been finished, I will kick off the release process.

[wgtmac]

@zhangjiashen This can be rebased to adopt parquet-format 2.10.0

[zhangjiashen]

@zhangjiashen This can be rebased to adopt parquet-format 2.10.0

@wgtmac I just rebased with master branch and please help take a look when you get a chance?

[wgtmac]

Could you please rebase it?

[zhangjiashen]

Could you please rebase it?

Rebased, can you help merge this PR?

[wgtmac]

+1

Thanks @zhangjiashen for working on this!

[wgtmac]

BTW, it would be good to add an interoperability test (as a follow-up PR) to read parquet files from here: apache/parquet-testing@da467da. You may want to take a look at this example: https://github.com/apache/parquet-mr/blob/44b56225be6fe7b74667f4f2430326ef1f076cc5/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/codec/TestInteropReadLz4RawCodec.java#L40

[wgtmac]

Will merge this by the end of this week if there is no objection.

[benibus]

Thanks @zhangjiashen for your persistence on this! In case I didn't already make this clear, there were a lot of eyes on this project and your contributions were crucial to its success - so thank you for stepping up here.

[wgtmac]

Merged this. Thanks @zhangjiashen and everyone!

[pitrou]

Is it planned to add the interoperability test mentioned in #1142 (comment) ?

[wgtmac]

Is it planned to add the interoperability test mentioned in #1142 (comment) ?

I think so. @pitrou

[zhangjiashen]

Is it planned to add the interoperability test mentioned in #1142 (comment) ?

I plan to add some interoperability tests.

[zhangjiashen]

Is it planned to add the interoperability test mentioned in #1142 (comment) ?

I think so. @pitrou

Added interOp tests in #1235