Skip to content

Conversation

@chenghao-intel
Copy link
Contributor

In jdk9, probably we can use the JDK API directly for the binary comparison.
https://bugs.openjdk.java.net/browse/JDK-8033148

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to cache the comparator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not necessary, as lexicographicalComparator() used a static compactor instance internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's a static final, the function call here should be very easy to be inlined in JIT I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

use SignedBytes.lexicographicalComparator().compare(x, y) instead.

@SparkQA
Copy link

SparkQA commented Aug 20, 2015

Test build #41312 has finished for PR 8335 at commit f846df5.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@adrian-wang
Copy link
Contributor

I read the code and find the comparator used here is not the same as we previously implemented.
For UnsignedBytes

public static int compare(byte a, byte b) {
  return toInt(a) - toInt(b);
}
public static int toInt(byte value) {
  return value & UNSIGNED_MASK;
}
private static final int UNSIGNED_MASK = 0xFF;

While for java.lang.Byte:

public static int compare(byte x, byte y) {
    return x - y;
}

@adrian-wang
Copy link
Contributor

so let's try

SignedBytes.lexicographicalComparator().compare(x, y)

@chenghao-intel
Copy link
Contributor Author

Yes, true, I thought all of the binary sorting result will be changed once we update this function, however, seems not. Probably some binary comparison code somewhere else.

@SparkQA
Copy link

SparkQA commented Aug 21, 2015

Test build #41354 has finished for PR 8335 at commit 73b9393.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@chenghao-intel
Copy link
Contributor Author

Closing the Guava(SignedBytes) has exactly the same implementation as current master code.

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.

4 participants