jonathanswenson commented on issue #22932: URL: https://github.com/apache/arrow/issues/22932#issuecomment-1434072725
I'm interested in helping integrate a correct decimal -> string implementation. I'm using the current bignum -> string implementation here from bn.ts https://github.com/apache/arrow/blob/a2881a124339d7d50088c5b9778c725316a7003e/js/src/util/bn.ts#L106-L123 But wrapping the implementation with some extra steps. + detect if the bigint is negative + flip the decimal + render the flipped decimal as a bigint (using [this implementation](https://github.com/apache/arrow/blob/a2881a124339d7d50088c5b9778c725316a7003e/js/src/util/bn.ts#L106-L123)) + add scale (place the decimal point in the correct location) + add a `-` if we flipped the decimal. Implementation of the negative check (I haven't verified it works with decimal 256) based on [c++ implementation](https://github.com/apache/arrow/blob/39bad544/cpp/src/arrow/util/basic_decimal.h#L125) ``` // check if the stored bigint is negative. // based on implementation here from arrow: // https://github.com/apache/arrow/blob/39bad544/cpp/src/arrow/util/basic_decimal.h#L125 // TODO will just using array.length - 1 work for 128 and 256bit decimals? const highOrderWord = new Int32Array([array[array.length - 1]])[0] return highOrderWord >= 0 ``` Implementation to flip the bigint to the equivalent "negative" value based on [c++ implementation](https://github.com/apache/arrow/blob/39bad544/cpp/src/arrow/util/basic_decimal.cc#L1144) ``` // based on implementation here from arrow: // https://github.com/apache/arrow/blob/39bad544/cpp/src/arrow/util/basic_decimal.cc#L1144 // idea is to flip the bits to take the 2's complement of the number to get the negative representation of the // 128 (or theoretically 256) bit integer. let carry = 1 for (let i = 0; i < array.length; i++) { const elem = array[i] const updated = ~elem + carry array.set([updated], i) carry &= elem === 0 ? 1 : 0 } ``` I don't quite have an efficient, strictly correct implementation of the decimal placement, but I think it shouldn't be too difficult to do correctly. I'm currently stripping off additional trailing zeros (just the way that we want to render these decimals) so `0.0001000` with scale of 7 => `0.0001` technically a scale of 4 It likely requires some changes to the the way that types are being dealt with (because a decimal is really just a bignum with a scale) but we'll need that scale to correctly do the render. Haven't spent too much time studying the decimalToString implementation, but I bet that with the scale the rendering of the value with the decimal point could be done in a single pass without needing to compose / concat / join extra string values. -- 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]
