Spec: add sort order to spec

[yyanyy]

Add sort order descriptions to the spec

[rdblue]

@aokolnychyi, do you think that sort order information should be optional in v2?

[rdblue]

I think that we need to clarify this a bit more. Sort order is optional when v2 is reading because v1 metadata is still valid. But a v2 writer must always write the sort-orders and default-sort-order-id fields. I think we need to change this to required for v2 and make sure there is text stating that these requirements are for writers and that v2 readers must accept all valid v1 metadata.

[yyanyy]

I think the last sentence in the current formatv2 section basically coverers the v2 readers accepting valid v1 metadata part. I'll add sort-orders and default-sort-order-id to the list of this section for now, and do you think if we should move this section/add more explanation about this in other places?

[rdblue]

Yes, since that section is intended to clarify, not specify. That doesn't need to be part of this PR, though.

[jackye1995]

how is NaN ordered? In java, Double.NaN is considered to be equal to itself and greater than all other double values including Double.POSITIVE_INFINITY. If this is the definition we follow, we should document it here. Otherwise something similar like a NaNOrder class should be defined in sort order.

[yyanyy]

Thanks, I think this is a good point I missed. I'm hoping to treat NaN as the largest value by default, but I'm not familiar with engines' implementations to say if it's something that could easily be achieved without much effort. @rdblue @aokolnychyi Do you have any recommendation to this? Thanks!

[rdblue]

The Float.compare implementation in Java will sort NaN larger than any other value and -NaN smaller than any other value. I think we should go with that.

    public static int compare(float f1, float f2) {
        if (f1 < f2)
            return -1;           // Neither val is NaN, thisVal is smaller
        if (f1 > f2)
            return 1;            // Neither val is NaN, thisVal is larger

        // Cannot use floatToRawIntBits because of possibility of NaNs.
        int thisBits    = Float.floatToIntBits(f1);
        int anotherBits = Float.floatToIntBits(f2);

        return (thisBits == anotherBits ?  0 : // Values are equal
                (thisBits < anotherBits ? -1 : // (-0.0, 0.0) or (!NaN, NaN)
                 1));                          // (0.0, -0.0) or (NaN, !NaN)
    }

I think that produces: -NaN < -Infinity < -value < -0 < 0 < value < Infinity < NaN

[jackye1995]

Yeah this order sounds good to me

[rdblue]

A bit more on this: the IEEE 754 spec is quoted in this Rust issue: rust-lang/rust#5585

It confirms that -NaN should sort before all values, and +NaN should sort after all values. So we can note in the doc that floating point ordering should follow the IEEE 754 totalOrder predicate.

[rdblue]

Looks good!

[rdblue]

Thanks @yyanyy! This looks very close to being ready.

[rdblue]

Merged! Thanks @yyanyy! And thanks to @jackye1995 for helping to review!

[findepi]

The -NaN is a bit ambiguous:

• it can be read as a result of applying unary minus to a NaN value. In Java, -Double.NaN is not distinguishable from Double.NaN (is exact same value bitwise).
  • my JVM returns true from Double.doubleToRawLongBits(-Double.NaN) == Double.doubleToRawLongBits(Double.NaN)
• it can be read as a "a IEEE 754 NaN value that has a sign bit set". For exampe
  • for example, in my JVM, Double.longBitsToDouble(0xfff8000000000000L) is such value, if I read this correctly. and so, Double.isNaN(Double.longBitsToDouble(0xfff8000000000000L)) is true (while also Double.doubleToRawLongBits(Double.longBitsToDouble(0xfff8000000000000L)) != Double.doubleToRawLongBits(Double.NaN) is true, i.e. this expression constructs a NaN value that;s bitwise distinguishable from Double.NaN)

In both cases however, "Java floating-point types comparisons" seems to sort all NaN values as peers, and "greater" than positive infinity:

System.out.println(Double.compare(Double.POSITIVE_INFINITY, Double.NaN)); // -1
System.out.println(Double.compare(Double.NaN, -Double.NaN)); // 0
System.out.println(Double.compare(Double.NaN, Double.longBitsToDouble(0xfff8000000000000L))); // 0
System.out.println(Double.compare(Double.NaN, Double.longBitsToDouble(0xfff8000012340000L))); // 0

[findepi]

if you want to tinker with this further, you can find https://github.com/findepi/urandom-bits/blob/417d908ebf5ef43f2d0c5187a5d8914b0139d8f6/src/main/java/findepi/java/NanOrder.java a useful starting point.

[findepi]

if your thinking matches mine, here is the PR #2891

[yyanyy]

Thanks for the input! You are right that in Java -Double.NaN equals Double.NaN so I think we should drop the mentioning of -NaN in spec. For the interpretation of having the sign bit set, I did some quick reading and it seems like NaN can be represented by a lot of ways (from here it says anything that looks like x111 1111 1axx xxxx xxxx xxxx xxxx xxxx where x means anything would be NaN), and the sign bit for NaN seems to have no meaning. In this case I think we probably want to avoid mentioning -NaN in spec completely.

[findepi]

@yyanyy you're right -- from representation perspective, NaN value still has a sign bit that can be set or unset, but this indeed has no meaning, at least in Java