NumericDecodeUtils.decodeBinary(byteBuf) returns wrong result for BigDecimal between 1 and -1

[alphaville76]

Bug Report

Versions

• Driver: master
• Database:
• Java: 11
• OS: Linux

Current Behavior

The BigDecimal 0.99 is decoded as 9900.00. The problem occurs for each big decimal between -1 and 1.
In the other cases works correctly.

Steps to reproduce

To reproduce, please run
short[] array = new short[]{1, -1, 0, 2, 9900}

        ByteBuf byteBuf = TestByteBufAllocator.TEST.buffer();
        //BigDecimal 0.99
        short[] array = new short[]{1, -1, 0, 2, 9900};
        for (int i = 0; i < array.length; i++) {
            byteBuf.writeShort(array[i]);
        }

        BigDecimal bigDecimal = NumericDecodeUtils.decodeBinary(byteBuf);

        System.out.println(bigDecimal);

Expected behavior/code

0.99 istead of 9900.00

Possible Solution

The problem is that a negative weight is not handled.
As stated in https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/numeric.c#L263

Note: the first digit of a NumericVar's value is assumed to be multiplied
by NBASE ** weight. Another way to say it is that there are weight+1
digits before the decimal point. It is possible to have weight < 0.

[alphaville76]

One can give a look to the jdbc version to understand how to handle a negative weight:
https://github.com/pgjdbc/pgjdbc/blob/a29767eaa27b97a68c21dee04e8483872783f2e2/pgjdbc/src/main/java/org/postgresql/util/ByteConverter.java#L184

I forgot to say that the bug occurs only with FORMAT_BINARY

[alphaville76]

possible fix, not sure, not tested, if it works when scale > 4

issue_556.patch.txt

[alphaville76]

Based on my understanding of the algorithm descripted in this paper, the same used by postgres as documented in numeric.c, I implemented decodeBinary from scratch as follows:

    private BigDecimal decodeBinary(ByteBuf byteBuf) {
        short numOfDigits = byteBuf.readShort();
        if (numOfDigits == 0) {
            return BigDecimal.ZERO;
        }
        short weight = byteBuf.readShort();
        short sign = byteBuf.readShort();
        short scale = byteBuf.readShort();
        short[] digits = new short[numOfDigits];
        for (short i = 0; i < numOfDigits; i++) {
            digits[i] = byteBuf.readShort();
        }

        StringBuilder sb = new StringBuilder();
        if (sign != 0) {
            sb.append("-");
        }
        sb.append("0.");
        for (short digit: digits) {
            sb.append(String.format("%04d", digit));
        }

        return new BigDecimal(sb.toString())
            .movePointRight((weight + 1) * 4)
            .setScale(scale, RoundingMode.DOWN);
    }

The unit-tests below runs sucessfully:

    @Test
    void decodeBinary2() {
        assertThat(decodeBinary(fromArray(new short[]{1, -1, 0, 2, 700})))
            .isEqualTo(new BigDecimal("0.07"));

        assertThat(decodeBinary(fromArray(new short[]{2, 0, 0, 2, 123, 4500})))
            .isEqualTo(new BigDecimal("123.45"));

        assertThat(decodeBinary(fromArray(new short[]{4, 2, 0, 2, 12, 3456, 7890, 1200})))
            .isEqualTo(new BigDecimal("1234567890.12"));

        assertThat(decodeBinary(fromArray(new short[]{1, -1, 0, 2, 9900})))
            .isEqualTo(new BigDecimal("0.99"));

        assertThat(decodeBinary(fromArray(new short[]{4, 0, -1, 12, 3, 1415, 9265, 3590})))
            .isEqualTo(new BigDecimal("-3.141592653590"));
    }

    private ByteBuf fromArray(short[] array) {
        ByteBuf byteBuf = TestByteBufAllocator.TEST.buffer();
        for (int i = 0; i < array.length; i++) {
            byteBuf.writeShort(array[i]);
        }
        return byteBuf;
    }

Looking forward for you feedback and hope my suggestion can help fixing the bug.

[mp911de]

Thanks for reporting the issue and your effort to provide a fix. I confirm that this is a bug that we haven't caught because we didn't had it tested with the appropriate data. Are you interested in submitting a pull request? If so, we have release week this week so we can include a fix.

[alphaville76]

Yes, I've just submitted the pull request.
Hopefully it can be included in this week release!