[jira] [Commented] (ARROW-6575) [JS] decimal toString does not support negative values

["Jonathan Swenson (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ARROW-6575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17621431#comment-17621431 ] 

Jonathan Swenson commented on ARROW-6575:
-----------------------------------------

Is this the correct method to use? It appears as though this still does not support negative numbers, and perhaps also does not support non-zero scale. 

From the implementation in the c++ source, I believe there are two missing pieces. 

+ handling of negative values. (determine if negative and negate before rendering) – [[determine if negative|https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/basic_decimal.h#L125]][[negation|https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/basic_decimal.cc#L377-L385]] 

+ scaling values appropriately (insert the decimal place / prepend leading zeros as necessary). [[implementation|https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/decimal.cc#L328-L386]]

 

The conversion to integer string is implemented in c++ [[here|https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/decimal.cc#L685-L698]] which includes checking if negative before rendering. 

 

I see values like:

+  101 (with scale 10, precision 13) renders as `1010000000000` – where I expect this as `101.0000000000`

+ -101 (with scale 10, precision 13) renders as `340282366920938500000000000000000000000`- I expect this as `-101.0000000000`

 

I'm not really a Javascript expert, but a similar approach to the negation check and flipping appears to work, but I'm fairly confident I'm missing some edge cases. 

 

General algorithm:

+ check to see if the high bits are negative 

+ if so, number is negative (prepend with "-") and add 1 to each "chunk" and handle overflows appropriately. 

+ render the string using current implementation. 

+ place decimal place (or prepend with 0s) based on the scale. 

> [JS] decimal toString does not support negative values
> ------------------------------------------------------
>
>                 Key: ARROW-6575
>                 URL: https://issues.apache.org/jira/browse/ARROW-6575
>             Project: Apache Arrow
>          Issue Type: Bug
>          Components: JavaScript
>    Affects Versions: 0.14.1
>            Reporter: Andong Zhan
>            Priority: Critical
>
> The main description is here: [https://github.com/apache/arrow/issues/5397]
> Also, I have a simple test case (slightly changed generate-test-data.js and generated-data-validators):
> {code:java}
> export const decimal = (length = 2, nullCount = length * 0.2 | 0, scale = 0, precision = 38) => vectorGenerator.visit(new Decimal(scale, precision), length, nullCount);
> function fillDecimal(length: number) {
>     // const BPE = Uint32Array.BYTES_PER_ELEMENT; // 4
>     const array = new Uint32Array(length);
>     // const max = (2 ** (8 * BPE)) - 1;
>     // for (let i = -1; ++i < length; array[i] = rand() * max * (rand() > 0.5 ? -1 : 1));
>     array[0] = 0;
>     array[1] = 1286889712;
>     array[2] = 2218195178;
>     array[3] = 4282345521;
>     array[4] = 0;
>     array[5] = 16004768;
>     array[6] = 3587851993;
>     array[7] = 126217744;
>     return array;
> }
> {code}
> and the expected value should be
> {code:java}
> expect(vector.get(0).toString()).toBe('-1000000000000000000000000000000000000');
> expect(vector.get(1).toString()).toBe('1000000000000000000000000000000000000');
> {code}
> However, the actual first value is 339282366920938463463374607431768211456 which is wrong! The second value is correct by the way.
> I believe the bug is in the function called 
> function decimalToString<T extends BN<BigNumArray>>(a: T) because it cannot return a negative value at all.
> [arrow/js/src/util/bn.ts|https://github.com/apache/arrow/blob/d54425de19b7dbb2764a40355d76d1c785cf64ec/js/src/util/bn.ts#L99]
> Line 99 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)