uros-b commented on code in PR #17628:
URL: https://github.com/apache/iceberg/pull/17628#discussion_r3773346879
##########
core/src/test/java/org/apache/iceberg/util/TestZOrderByteUtil.java:
##########
@@ -379,6 +382,97 @@ public void testByteTruncateOrFill() {
}
}
+ /**
+ * The random float/double ordering tests above draw from {@code
nextFloat()}/{@code
+ * nextDouble()}, so the compared values essentially never agree on their
high bits. Ordering is
+ * then decided by the high bits alone, which hides any corruption of the
low bits. These cases
+ * pin the ordering for values that differ *only* in low mantissa bits,
where the low bytes are
+ * what decides the comparison.
+ */
+ @Test
+ public void testFloatOrderingForConsecutiveMantissaValues() {
+ int baseBits = Float.floatToIntBits(1.0f);
+ List<Float> values = Lists.newArrayList();
+ for (int i = 0; i < 64; i++) {
+ values.add(Float.intBitsToFloat(baseBits + i));
+ }
+
+ assertOrderPreserved(
+ values, value -> encode(buffer ->
ZOrderByteUtils.floatToOrderedBytes(value, buffer)));
+ }
+
+ @Test
+ public void testDoubleOrderingForValuesDifferingInLowMantissaBits() {
+ List<Double> values = Lists.newArrayList();
+ for (int i = 0; i < 64; i++) {
+ values.add(1.0d + (i * 0x1.0p-30));
+ }
+
+ assertOrderPreserved(
+ values, value -> encode(buffer ->
ZOrderByteUtils.doubleToOrderedBytes(value, buffer)));
+ }
+
+ @Test
+ public void testNegativeDoubleOrderingForValuesDifferingInLowMantissaBits() {
+ List<Double> values = Lists.newArrayList();
+ for (int i = 63; i >= 0; i--) {
+ values.add(-1.0d - (i * 0x1.0p-30));
+ }
+
+ assertOrderPreserved(
+ values, value -> encode(buffer ->
ZOrderByteUtils.doubleToOrderedBytes(value, buffer)));
+ }
+
+ /** Boundary pairs, including the ones that straddle zero and the extremes
of the range. */
+ @Test
+ public void testDoubleOrderingForBoundaryPairs() {
+ double[][] ascendingPairs = {
+ {-921614.125d, -921614.0625d},
+ {-1.6001329423771755E213d, -1.600132804916327E213d},
+ {5.716890676284865E-207d, 5.7168911255697246E-207d},
+ {-Double.MIN_VALUE, 0.0d},
+ {0.0d, Double.MIN_VALUE},
+ {-Double.MAX_VALUE, Double.MAX_VALUE},
+ {-1.0d, 1.0d},
+ };
+
+ for (double[] pair : ascendingPairs) {
+ assertOrderPreserved(
+ Lists.newArrayList(pair[0], pair[1]),
+ value -> encode(buffer ->
ZOrderByteUtils.doubleToOrderedBytes(value, buffer)));
+ }
+ }
+
+ private static byte[] encode(Function<ByteBuffer, ByteBuffer> encoder) {
+ return
encoder.apply(ZOrderByteUtils.allocatePrimitiveBuffer()).array().clone();
+ }
+
+ /**
+ * Asserts that the given values, which must already be in ascending order,
encode to
+ * lexicographically ascending bytes.
+ */
+ private static <T extends Comparable<T>> void assertOrderPreserved(
+ List<T> ascendingValues, Function<T, byte[]> encoder) {
+ for (int i = 1; i < ascendingValues.size(); i++) {
+ T smaller = ascendingValues.get(i - 1);
+ T larger = ascendingValues.get(i);
+ assertThat(smaller).isLessThan(larger);
+
+ byte[] smallerBytes = encoder.apply(smaller);
+ byte[] largerBytes = encoder.apply(larger);
+
+
assertThat(UnsignedBytes.lexicographicalComparator().compare(smallerBytes,
largerBytes))
+ .as(
+ "Ordering of %s should match ordering of bytes, %s -> %s is not
less than %s -> %s",
+ smaller.getClass().getSimpleName(),
+ smaller,
+ Arrays.toString(smallerBytes),
+ larger,
+ Arrays.toString(largerBytes))
+ .isNegative();
+ }
+ }
+
Review Comment:
Please note an IEEE-754 edge-coverage gap here. No test pins {-0.0d, +0.0d}
or {Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY}. The corrected mask
orders all of them correctly (-0.0d -> 0x7FFFFFFFFFFFFFFF, +0.0d ->
0x8000000000000000; -Inf -> 0x000FFFFFFFFFFFFF, +Inf -> 0xFFF0000000000000),
but for a change whose whole purpose is to close a systematic ordering gap,
pinning these edges would complete coverage. Also, it would be nice to add some
NaN tests too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
