Issue 6091: Remove Facebook collections library #6129

rtjd6554 · 2025-11-27T14:32:43Z

Make sure you have checked all steps below.

Issue

My PR fully resolves the following issues. I've referenced an issue in the PR title, for example "Issue 1234 - My
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
- Resolves Remove Facebook collections library #6091

Tests

My PR adds the following tests based on our test strategy OR does not need testing for this extremely good reason:
- Existing execution of test suite
- New tests for classes created: ByteArrayTest

Documentation

In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
separate issue for that below.
If I have added new Java code, I have added Javadoc that explains it following our conventions and style.
If I have added or removed any dependencies from the project, I have updated the NOTICES file.

m09526 · 2025-11-28T09:21:47Z

Could we replace this with a commons lang3 Pair? They have ImmutablePair and MutablePair as required.

patchwork01 · 2025-11-28T09:48:56Z

Personally I'd rather avoid using a Pair-type data structure at all, in favour of something with a more descriptive name. Using a Java record lets you achieve that with very little boilerplate, something like this:

public record FieldValue<T>(String fieldName, T value) {
}

I'd keep that in the Athena code, next to where it's used, maybe nest it in the class and make it static.

…n of class

ca61688 · 2025-12-02T16:05:20Z

java/athena/src/main/java/sleeper/athena/record/FieldAtDimension.java

+ */
+package sleeper.athena.record;
+
+public record FieldAtDimension(int dimension, Object value) {


This is field AT dimension whereas other is field AS String. Is that correct?

Addressed typo

ca61688 · 2025-12-02T16:08:16Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+     *
+     * @param  array1 first ByteArray object
+     * @param  array2 second ByteArray object
+     * @return        true is equal


true if* equal

Addressed typo

ca61688 · 2025-12-02T16:08:53Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+     * Method for checking equality versus generic object declaration.
+     *
+     * @param  o object to compare versus
+     * @return   true is equal


true if* equal

Addressed typo

ca61688 · 2025-12-02T16:11:10Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ByteArrayTest {


Is it worth testing equals validation for one null one not and for the contents of the arrays being equal?

ca61688

Few small comments otherwise looks good

gaffer01 · 2025-12-03T09:54:33Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+    }
+
+    /**
+     * Method for checking equaility.


Typo on equaility - should be equality

patchwork01

Facebook have the same license as this project. I'm basing this review on my own reading of the terms:

https://github.com/facebookarchive/jcommon/blob/master/LICENSE

patchwork01 · 2025-12-03T09:09:44Z

NOTICES


-Facebook Jcommon collections (com.facebook.jcommon:collections:0.*)
-
- Apache License, Version 2.0


This is no longer a Maven dependency, but we've copied code directly from there so I think we need to state that in this file. I think we still need this notice, but in its own section explaining we copied the ByteArray class.

patchwork01 · 2025-12-03T09:16:23Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+/**
+ * Class to provide wrapper object for primitive byte array into an object.
+ * Require as hashCode and equals requied for usage of type within a HashSet.
+ *


There's an extra blank line here.

patchwork01 · 2025-12-03T09:26:32Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+
+/**
+ * Class to provide wrapper object for primitive byte array into an object.
+ * Require as hashCode and equals requied for usage of type within a HashSet.


Please see the Oracle Javadoc style guide:

https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html#styleguide

A class description shouldn't start with "class", see "Class/interface/field descriptions can omit the subject and simply state the object." If we take the first sentence as it is, it could start with "A wrapper for ...".

There's a typo at "requied".

There are grammatical problems at "for primitive", "Require as", "equals required", "usage of type".

It's not technically true that by wrapping a byte array we turn it into an object. A byte array is already an object.

I don't think HashSet is relevant to this class?

I think the important part for us is it implements Comparable and equals/hashCode.

Addressed,

disagree on the HashSet comment as otherwise we could just use the primitive here, it is directly because we use in within HashSets that we need it as a separate class.

patchwork01 · 2025-12-03T09:28:37Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+    public static final ByteArrayComparator BYTE_ARRAY_COMPARATOR = new ByteArrayComparator();
+
+    /**
+     * Returns contents of object as primitive type.


It's fine for this to be a phrase rather than a full sentence, but there are grammatical problems. It's missing "the", "this", "a". I'd put "Returns the contents of this object as a primitive byte array".

patchwork01 · 2025-12-03T09:44:03Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+    /**
+     * Returns contents of object as primitive type.
+     *
+     * @return array details in primitive type


I'm not sure what "details" or "in primitive type" means here. I'd just put "the byte array".

patchwork01 · 2025-12-03T10:03:25Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+    }
+
+    /**
+     * Method for checking equaility.


A method description shouldn't start with "method", see "Class/interface/field descriptions can omit the subject and simply state the object."

https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html#styleguide

There's also a typo at "equaility".

patchwork01 · 2025-12-03T10:04:51Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+     * @param  o object to compare versus
+     * @return   true if equal
+     */
+    public boolean equals(Object o) {


Usually for equals/hashCode/toString we use an @Override annotation and inherit the Javadoc from Object. The Javadoc you get when you just use @Override explains a bit more.

There are a number of instances of this in this file.

patchwork01 · 2025-12-03T10:11:27Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+/**
+ * Declared implementation of PureByteArray to match design for Arrays comparion.
+ */
+class PureByteArray extends ByteArray {


If we make this a nested static class as it was in the Facebook code, this can be private.

patchwork01 · 2025-12-03T10:11:34Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+/**
+ * Comparator for ByteArray type.
+ */
+class ByteArrayComparator implements Comparator<ByteArray> {


If we make this a nested static class as it was in the Facebook code, this can be private.

patchwork01 · 2025-12-03T10:13:01Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+}
+
+/**
+ * Declared implementation of PureByteArray to match design for Arrays comparion.


What is this trying to say?

There's a typo at "comparion".

patchwork01 · 2025-12-03T10:19:41Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+        ByteArray second = ByteArray.wrap(new byte[]{7, 8, 9});
+
+        // When / Then
+        assertThat(first.getArray()).isEqualTo(second.getArray());


This isn't testing comparison of the ByteArray objects, it's retrieving the arrays and asserting that those are equal.

I think we need to call first.equals(second) directly.

patchwork01 · 2025-12-03T10:19:59Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+        ByteArray second = ByteArray.wrap(data);
+
+        // Then
+        assertThat(first.getArray()).isEqualTo(second.getArray());


This isn't testing comparison of the ByteArray objects, it's retrieving the arrays and asserting that those are equal.

I think we need to call first.equals(second) directly.

patchwork01 · 2025-12-03T10:20:26Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+        ByteArray result = ByteArray.wrap(data);
+
+        // Then
+        assertThat(result.getArray()).isEqualTo(data);


Isn't this the same as the first test?

Addressed, was from time where first test was validation something different

patchwork01 · 2025-12-03T10:22:15Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    }
+
+    @Test
+    void shouldValidateEqualityWhenContentsOfArraysComeFromSameSource() {


This could be more specific than "validate equality", e.g. shouldFindByteArraysAreEqualWithSameSourceArray.

patchwork01 · 2025-12-03T10:24:55Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    }
+
+    @Test
+    void shouldCompareByteArraysCorrectly() {


This test name is too vague, it suggests it's asserting more than it is. I think you wanted to assert that when we wrap two separate arrays containing the same data, they are equal?

patchwork01 · 2025-12-03T10:25:16Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    }
+
+    @Test
+    void shouldValidateEqualsMethodWithNullObjects() {


A test name should describe the behaviour being asserted, rather than name the method being tested.

patchwork01 · 2025-12-03T10:25:53Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+        ByteArray valid = ByteArray.wrap(new byte[]{'a', 2, 3});
+
+        // When / Then
+        assertThat(ByteArray.equals(valid, null)).isFalse();


It looks like we need a version of these tests for the instance method as well?

I don't think we ever use ByteArray.equals(ByteArray, ByteArray), so it might be simpler to get rid of that and only keep the instance method?

patchwork01 · 2025-12-03T10:28:34Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    void shouldValidateEqualsMethodsWithDeclaredEmptyArrays() {
+        // Given
+        ByteArray first = ByteArray.wrap(null);
+        ByteArray second = ByteArray.wrap(null);


These aren't empty arrays, they're nulls.

patchwork01 · 2025-12-03T10:29:01Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    @Test
+    void shouldAllowUsageWithinAHashSet() {
+        //Give
+        TreeSet<ByteArray> treeSet = new TreeSet<>();


This isn't a HashSet, it's a TreeSet.

patchwork01 · 2025-12-03T10:29:47Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ByteArrayTest {


It doesn't look like we've got any tests for cases where the byte arrays aren't equal, or for using the comparator, e.g. for sorting.

6091: New pair class to remove facebook.collections usage

630e219

patchwork01 and others added 13 commits November 28, 2025 09:58

6091 Remove Pair

f6bb550

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

0f9c02e

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

f09aec3

6091: Move records out to seperate class files.

da1f344

6091: New Class for ByteArray

972cafa

6091: Adjust imports to replace facebook collections with core versio…

4b26524

…n of class

6091: Update test class

0bae30e

6091: Update pom declarations

0fab329

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

35524cf

6091: Update NOTICES

e2612aa

6091: Improve comparator

95d060b

6091: Access modifiers correction

d9f2036

6091: Update compare logic

73d4c64

rtjd6554 marked this pull request as ready for review December 2, 2025 15:34

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025

ca61688 self-assigned this Dec 2, 2025

ca61688 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025

ca61688 reviewed Dec 2, 2025

View reviewed changes

ca61688 requested changes Dec 2, 2025

View reviewed changes

ca61688 assigned rtjd6554 and unassigned ca61688 Dec 2, 2025

rtjd6554 added 3 commits December 3, 2025 08:07

6091: Correct class name typo

46b7450

6091: Correct java doc typo

d3cd782

6091: Additional unit test

6fa837a

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

d4fa64f

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 3, 2025

rtjd6554 removed their assignment Dec 3, 2025

ca61688 self-assigned this Dec 3, 2025

ca61688 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 3, 2025

ca61688 approved these changes Dec 3, 2025

View reviewed changes

ca61688 assigned rtjd6554 and unassigned ca61688 Dec 3, 2025

patchwork01 added the on-hold label Dec 3, 2025

6091: Update copyright declaration

1e019bb

gaffer01 reviewed Dec 3, 2025

View reviewed changes

patchwork01 requested changes Dec 3, 2025

View reviewed changes

rtjd6554 added 9 commits December 3, 2025 10:45

6091: Typos and Javadoc corrections

1dc5db4

6091: Rename class and Javadoc

1e9e865

6091: MIssing Javadoc

2fb1a8f

6091: Javadoc improvements

764cbbf

6091: Update unit tests

8729321

6091: Checkstyle updates

837a218

6091: Update copyright

87aa5c2

6091: Update copyright

e1cc495

6091: Update copyright

5908008


		import static org.assertj.core.api.Assertions.assertThat;

		public class ByteArrayTest {


		Facebook Jcommon collections (com.facebook.jcommon:collections:0.*)

		- Apache License, Version 2.0

Issue 6091: Remove Facebook collections library #6129

Are you sure you want to change the base?

Issue 6091: Remove Facebook collections library #6129

Conversation

rtjd6554 commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tests

Documentation

Uh oh!

m09526 commented Nov 28, 2025

Uh oh!

patchwork01 commented Nov 28, 2025

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!

ca61688 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!

patchwork01 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!

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!

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!

rtjd6554 commented Nov 27, 2025 •

edited

Loading