GH-39680: [Java] enable half float support on Java module by davisusanibar · Pull Request #39681 · apache/arrow

davisusanibar · 2024-01-18T02:55:00Z

Rationale for this change

To enable half float support on Java module.

What changes are included in this PR?

Add initial Float16 type support
Unit test
Integration test
Documentation

Are these changes tested?

Yes.

Are there any user-facing changes?

No

Closes: [Java] Add Float16 type support #39680

github-actions · 2024-01-18T02:55:29Z

⚠️ GitHub issue #39680 has been automatically assigned in GitHub to PR creator.

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java

pitrou · 2024-01-22T15:35:07Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java

+   *              where the value will be read from
+   * @return 4 byte float value
+   */
+  public short getFloat16(long index) {


Given that setFloat16 takes a float, by symmetry this method should probably return a float too.
@danepitkin WDYT?

Casting to float will involve a conversion to a 32 bit type. I think it would be nice to provide set/get operations on short as the core APIs and set/get operations on float as helper APIs. Thoughts?

Well, getShort and setShort already seems provided?

heh, good point.

I agree it makes sense to change this to float.

jduo · 2024-01-22T20:22:20Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java

+  // Positive infinity of type half-precision float.
+  private static final short POSITIVE_INFINITY = (short) 0x7c00;
+  // A Not-a-Number representation of a half-precision float.
+  private static final short NaN = (short) 0x7e00;


Should this and POSITIVE_INFINTY be public for app users to make use of?

jduo · 2024-01-22T20:25:04Z

java/vector/src/main/codegen/templates/UnionReader.java

 public class UnionReader extends AbstractFieldReader {

-  private BaseReader[] readers = new BaseReader[45];
+  private BaseReader[] readers = new BaseReader[46];


It'd be good if this constant was given a name.

…dataset module for Float16

davisusanibar · 2024-01-25T05:18:41Z

@github-actions crossbow submit java-jars

github-actions · 2024-01-25T05:19:14Z

Only contributors can submit requests to this bot. Please ask someone from the community for help with getting the first commit in.
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/7650222562

davisusanibar · 2024-01-25T05:20:40Z

In order to test Float16 data types in Arrow Java Dataset module, apache/arrow-testing#99 is required.

lidavidm

In general I think I'd favor using short as the 'default' float16 type and having float be the 'special'. Else the 'default' case has ambiguity when going from float -> float16.

lidavidm · 2024-01-25T18:20:41Z

java/dataset/src/test/java/org/apache/arrow/dataset/TestAllTypes.java

+  /*
+  The purpose of this method is to refresh the data in alltypes-java.parquet as required:
+  https://github.com/apache/arrow-testing/blob/master/data/parquet/alltypes-java.parquet
+   */
+  public static void main(String[] args) throws Exception {
+    TestAllTypes test = new TestAllTypes();
+    try (BufferAllocator allocator = new RootAllocator();
+         VectorSchemaRoot root = test.generateAllTypesVector(allocator)) {
+      byte[] featherData = test.serializeFile(root);
+      try (SeekableByteChannel channel = new ByteArrayReadableSeekableByteChannel(featherData);
+           ArrowStreamReader reader = new ArrowStreamReader(channel, allocator)) {
+        TMP.create();
+        final File writtenFolder = TMP.newFolder();
+        final String writtenParquet = writtenFolder.toURI().toString();
+        DatasetFileWriter.write(allocator, reader, FileFormat.PARQUET,
+                writtenParquet);
+        Objects.requireNonNull(writtenFolder.listFiles());
+        Files.move(writtenFolder.listFiles()[0].toPath(),
+                Paths.get(writtenFolder.toPath().toString(), "alltypes-java.parquet"));
+        System.out.println("The file data/parquet/alltypes-java.parquet should be updated with this new data: " +
+                writtenFolder.listFiles()[0].toURI());
+      }
+    }
+  }


Can you refactor this so that it doesn't duplicate the test itself? Also, technically this isn't really necessary; you can run the test in an IDE (and set a breakpoint so you can copy the temporary file)

Sure, I will clean and delete this section.

lidavidm · 2024-01-25T18:21:40Z

java/dataset/pom.xml

+                <configuration>
+                    <enableAssertions>false</enableAssertions>
+                    <systemPropertyVariables>
+                        <arrow.test.dataRoot>${project.basedir}/../../testing/data</arrow.test.dataRoot>


Where is this actually used? I don't see any new tests referencing it.

The redundant part has already been added before, let me remove it

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java

lidavidm · 2024-01-25T18:22:55Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java

+   *              where the value will be written
+   * @param value value to write
+   */
+  public void setFloat16(long index, float value) {


IMO, these methods should work via shorts and not floats, since there is no conversion ambiguity with shorts.

lidavidm · 2024-01-25T18:23:33Z

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java

    }
  }

+  @Test /* Float2Vector */


redundant comment?

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java

lidavidm · 2024-01-25T18:24:17Z

java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestArrowBuf.java

+    ) {
+      buf.setFloat16(0, +32.875f);
+      assertEquals((short) 0x501c, Float16.toFloat16(+32.875f));
+      assertEquals(Float16.toFloat((short) 0x501c), buf.getFloat16(0), 0);


Yeah, we should just use short in the first place.

…tAllTypes (#99) To be able to test Float16 data types into Arrow Java Dataset module. To fix dataset errors at: apache/arrow#39681

lidavidm · 2024-01-25T18:27:55Z

I've merged the testing PR.

lidavidm · 2024-01-30T13:31:43Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java

+
+/**
+ * Lifted from Apache Parquet MR project:
+ * https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/schema/Float16.java


Er, use the URL that contains the commit hash, not 'main'

davisusanibar · 2024-01-31T18:29:30Z

Just for the record, I will work on these items in another PR:

Testing the C data interface with Float16 data
Add support for VectorVisitor classes (compare/validate)
Add support for Float16 to the arrow-algorithm module

If there are any other items that need to be reviewed, please let me know.

cc @lidavidm @danepitkin @vibhatha

lidavidm · 2024-01-31T18:38:37Z

Do we have IPC integration tests for float16? I don't obviously see any

pitrou · 2024-01-31T18:39:54Z

Do we have IPC integration tests for float16? I don't obviously see any

If not, then perhaps it's worth opening a GH issue?

conbench-apache-arrow · 2024-01-31T21:05:33Z

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

There were no benchmark performance regressions. 🎉

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

lidavidm · 2024-02-01T17:38:49Z

Do we have IPC integration tests for float16? I don't obviously see any

@davisusanibar can you follow up here?

…he#39681) ### Rationale for this change - To enable half float support on Java module. ### What changes are included in this PR? - [x] Add initial Float16 type support - [x] Unit test - [x] Integration test - [x] Documentation ### Are these changes tested? Yes. ### Are there any user-facing changes? No * Closes: apache#39680 Authored-by: david dali susanibar arce <davi.sarces@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

vibhatha · 2024-03-19T08:20:16Z

@github-actions crossbow submit -g java

github-actions · 2024-03-19T08:22:42Z

Revision: 19869ad

Submitted crossbow builds: ursacomputing/crossbow @ actions-f0517a91b4

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

vibhatha · 2024-03-19T15:23:02Z

@github-actions crossbow submit java-jars

github-actions · 2024-03-19T15:25:18Z

Revision: 19869ad

Submitted crossbow builds: ursacomputing/crossbow @ actions-62925ab8ab

Task	Status
java-jars

…he#39681) ### Rationale for this change - To enable half float support on Java module. ### What changes are included in this PR? - [x] Add initial Float16 type support - [x] Unit test - [x] Integration test - [x] Documentation ### Are these changes tested? Yes. ### Are there any user-facing changes? No * Closes: apache#39680 Authored-by: david dali susanibar arce <davi.sarces@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

feat: initial draft to enable half float support

a2f4356

github-actions bot added Component: Java awaiting review Awaiting review labels Jan 18, 2024

davisusanibar commented Jan 18, 2024

View reviewed changes

java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/Float16.java Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 18, 2024

pitrou reviewed Jan 22, 2024

View reviewed changes

jduo reviewed Jan 22, 2024

View reviewed changes

davisusanibar mentioned this pull request Jan 24, 2024

GH-39681: [Java] Adding Float16 for testing the Java dataset test TestAllTypes apache/arrow-testing#99

Merged

davisusanibar added 2 commits January 24, 2024 09:11

fix: Change short to float into get/set Float16 operations + Testing …

05646b8

…dataset module for Float16

feat: clean code + update documentation on java side for float16 support

1225569

github-actions bot added the Component: Documentation label Jan 25, 2024

davisusanibar added 2 commits January 24, 2024 23:42

Merge branch 'main' into apacheGH-39680-float16

196bc16

fix: update notes numbers order

8e41813

davisusanibar marked this pull request as ready for review January 25, 2024 05:38

davisusanibar requested a review from lidavidm as a code owner January 25, 2024 05:38

lidavidm reviewed Jan 25, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jan 25, 2024

davisusanibar added 2 commits January 25, 2024 19:03

Merge branch 'main' into apacheGH-39680-float16

4f47247

fix: clean codee

fb34aac

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 26, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 29, 2024

lidavidm reviewed Jan 30, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 30, 2024

fix: adding commit hash for lifted code

19869ad

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 30, 2024

lidavidm approved these changes Jan 30, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 30, 2024

lidavidm merged commit 2a87693 into apache:main Jan 31, 2024

lidavidm removed the awaiting merge Awaiting merge label Jan 31, 2024

vibhatha mentioned this pull request Mar 19, 2024

[Java] Test failure in Dataset regarding TestAllTypes #40568

Closed

Conversation

davisusanibar commented Jan 18, 2024 • 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!

github-actions bot commented Jan 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davisusanibar commented Jan 25, 2024

Uh oh!

github-actions bot commented Jan 25, 2024

Uh oh!

davisusanibar commented Jan 25, 2024

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Jan 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davisusanibar commented Jan 31, 2024

Uh oh!

lidavidm commented Jan 31, 2024

Uh oh!

pitrou commented Jan 31, 2024

Uh oh!

conbench-apache-arrow bot commented Jan 31, 2024

Uh oh!

lidavidm commented Feb 1, 2024

Uh oh!

vibhatha commented Mar 19, 2024

Uh oh!

github-actions bot commented Mar 19, 2024

Uh oh!

vibhatha commented Mar 19, 2024

Uh oh!

davisusanibar commented Jan 18, 2024 •

edited

Loading