Skip to content

Conversation

@rtjd6554
Copy link
Collaborator

@rtjd6554 rtjd6554 commented Nov 27, 2025

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.

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
Copy link
Member

m09526 commented Nov 28, 2025

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

@patchwork01
Copy link
Collaborator

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.

@rtjd6554 rtjd6554 marked this pull request as ready for review December 2, 2025 15:34
@rtjd6554 rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025
@ca61688 ca61688 self-assigned this Dec 2, 2025
@ca61688 ca61688 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025
*/
package sleeper.athena.record;

public record FieldAtDimension(int dimension, Object value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

*
* @param array1 first ByteArray object
* @param array2 second ByteArray object
* @return true is equal
Copy link
Collaborator

Choose a reason for hiding this comment

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

true if* equal

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

Choose a reason for hiding this comment

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

true if* equal


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

public class ByteArrayTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@ca61688 ca61688 left a comment

Choose a reason for hiding this comment

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

Few small comments otherwise looks good

@ca61688 ca61688 assigned rtjd6554 and unassigned ca61688 Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove Facebook collections library

5 participants