You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "dioptre (via GitHub)" <gi...@apache.org> on 2024/03/22 01:26:33 UTC
[PR] [JS] Critical Fix - Fixed decimal conversions in js [arrow]
dioptre opened a new pull request, #40729:
URL: https://github.com/apache/arrow/pull/40729
### Rationale for this change
Fixes https://github.com/apache/arrow/issues/37920
Decimals are broken - need a correct way to convert decimals to numbers in js
Also included an option to include a denominator (BigInt(1/scale)) as scale is part of the metadata
### What changes are included in this PR?
Submitting a correct way to convert decimals to numbers
### Are these changes tested?
Yes, includes a test
### Are there any user-facing changes?
No
**This PR contains a "Critical Fix".**
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015889873
Updated the fix issue above, kept a reference ^^^
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1536556864
##########
js/src/util/bn.ts:
##########
@@ -68,24 +68,36 @@ Object.assign(SignedBigNum.prototype, BigNum.prototype, { 'constructor': SignedB
Object.assign(UnsignedBigNum.prototype, BigNum.prototype, { 'constructor': UnsignedBigNum, 'signed': false, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
Object.assign(DecimalBigNum.prototype, BigNum.prototype, { 'constructor': DecimalBigNum, 'signed': true, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
+//FOR ES2020 COMPATIBILITY
+const TWO_TO_THE_64 = BigInt(4294967296) * BigInt(4294967296); // 2^64 = 0x10000000000000000n
+const TWO_TO_THE_64_MINUS_1 = TWO_TO_THE_64 - BigInt(1); // (2^32 * 2^32) - 1 = 0xFFFFFFFFFFFFFFFFn
+
/** @ignore */
-function bigNumToNumber<T extends BN<BigNumArray>>(bn: T) {
- const { buffer, byteOffset, length, 'signed': signed } = bn;
- const words = new BigUint64Array(buffer, byteOffset, length);
+export function bigNumToNumber<T extends BN<BigNumArray>>(bn: T, scale?: number) {
+ const { buffer, byteOffset, byteLength, 'signed': signed } = bn;
+ const words = new BigUint64Array(buffer, byteOffset, byteLength / 8);
const negative = signed && words.at(-1)! & (BigInt(1) << BigInt(63));
- let number = negative ? BigInt(1) : BigInt(0);
- let i = BigInt(0);
+ let number = BigInt(0);
+ let i = 0;
if (!negative) {
for (const word of words) {
- number += word * (BigInt(1) << (BigInt(32) * i++));
+ number |= word * (BigInt(1) << BigInt(64 * i++));
}
} else {
for (const word of words) {
- number += ~word * (BigInt(1) << (BigInt(32) * i++));
+ number |= (word ^ TWO_TO_THE_64_MINUS_1) * (BigInt(1) << BigInt(64 * i++));
}
number *= BigInt(-1);
+ number -= BigInt(1);
+ }
+ if (scale) {
+ const denominator = BigInt(Math.pow(10, scale));
+ const quotient = number / denominator;
+ const remainder = number % denominator;
+ const n = Number(quotient) + (Number(remainder) / Number(denominator));
+ return n;
}
- return number;
+ return Number(number);
Review Comment:
Isn't this incorrect when we have larger numbers?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2021435114
No worries, this drove me nuts at first - I was expecting something more like this - https://en.wikipedia.org/wiki/Binary-coded_decimal
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2014184479
Thanks for the pull request.
This does not yet fix encoding so we cannot close https://github.com/apache/arrow/issues/37920.
```ts
arrow.vectorFromArray([1, 12, 34], new arrow.Decimal(3, 18))
```
Also, it seems like this still ignores the scale. If I try the example in https://github.com/apache/arrow/issues/37920#issue-1916512787, I still get 1000 instead of 1 (the scale is ignored).
```ts
const hex = 'FFFFFFFF780000001000000000000A000C000600050008000A000000000104000C000000080008000000040008000000040000000100000014000000100014000800060007000C00000010001000000000000107100000001C0000000400000000000000010000006400000008000C0004000800080000001200000003000000FFFFFFFF8800000014000000000000000C0016000600050008000C000C0000000003040018000000300000000000000000000A0018000C00040008000A0000003C00000010000000030000000000000000000000020000000000000000000000000000000000000000000000000000003000000000000000000000000100000003000000000000000000000000000000E8030000000000000000000000000000E02E0000000000000000000000000000D0840000000000000000000000000000FFFFFFFF00000000';
const bytes = Uint8Array.from(hex.match(/.{1,2}/g).map(s => parseInt(s, 16)));
const vec = arrow.tableFromIPC(bytes).getChild('d')!;
console.log(vec.get(0)) // [1000, 0, 0, 0] <- byte array looks correct
console.log(`${vec.get(0)}`) // "1000" <- incorrect, should be 1; ignores scale
console.log(Number(vec.get(0))) // 1000 <- incorrect, should be 1
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015954222
:warning: GitHub issue #40755 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2014236633
Are the tests off? I’ll take a look soon.
I made it so you pass the denominator through instead of scale. The reason
for this is that with 10^9 numbers to convert you don’t need to do a scale
to denominator conversion for each number. Passing the denominator as a
bigint instead will make the function much faster.
On Thu, Mar 21, 2024 at 19:27 Dominik Moritz ***@***.***>
wrote:
> vec.get(0).valueOf(1000n) return the correct value but it would be nice
> to pass the scale though. It's in the type but I don't know what is the
> best way to get that. This otherwise looks correct.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#issuecomment-2014197628>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGAX2QDJV7ECPJO3IW3YZOJHNAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJUGE4TONRSHA>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [JS] Critical Fix - Fixed decimal conversions in js [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2014149906
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2016371134
I took the literal definition bignum to number?
On Fri, Mar 22, 2024 at 21:14 Dominik Moritz ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In js/src/util/bn.ts
> <https://github.com/apache/arrow/pull/40729#discussion_r1536556864>:
>
> > }
> - return number;
> + return Number(number);
>
> Isn't this incorrect when we have larger numbers?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#pullrequestreview-1956298728>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGGRD74D7ATOE5L72U3YZT6QVAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJWGI4TQNZSHA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1537902839
##########
js/src/util/bn.ts:
##########
@@ -68,24 +68,36 @@ Object.assign(SignedBigNum.prototype, BigNum.prototype, { 'constructor': SignedB
Object.assign(UnsignedBigNum.prototype, BigNum.prototype, { 'constructor': UnsignedBigNum, 'signed': false, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
Object.assign(DecimalBigNum.prototype, BigNum.prototype, { 'constructor': DecimalBigNum, 'signed': true, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
+//FOR ES2020 COMPATIBILITY
+const TWO_TO_THE_64 = BigInt(4294967296) * BigInt(4294967296); // 2^64 = 0x10000000000000000n
+const TWO_TO_THE_64_MINUS_1 = TWO_TO_THE_64 - BigInt(1); // (2^32 * 2^32) - 1 = 0xFFFFFFFFFFFFFFFFn
+
/** @ignore */
-function bigNumToNumber<T extends BN<BigNumArray>>(bn: T) {
- const { buffer, byteOffset, length, 'signed': signed } = bn;
- const words = new BigUint64Array(buffer, byteOffset, length);
+export function bigNumToNumber<T extends BN<BigNumArray>>(bn: T, scale?: number) {
+ const { buffer, byteOffset, byteLength, 'signed': signed } = bn;
+ const words = new BigUint64Array(buffer, byteOffset, byteLength / 8);
const negative = signed && words.at(-1)! & (BigInt(1) << BigInt(63));
- let number = negative ? BigInt(1) : BigInt(0);
- let i = BigInt(0);
+ let number = BigInt(0);
+ let i = 0;
if (!negative) {
for (const word of words) {
- number += word * (BigInt(1) << (BigInt(32) * i++));
+ number |= word * (BigInt(1) << BigInt(64 * i++));
}
} else {
for (const word of words) {
- number += ~word * (BigInt(1) << (BigInt(32) * i++));
+ number |= (word ^ TWO_TO_THE_64_MINUS_1) * (BigInt(1) << BigInt(64 * i++));
}
number *= BigInt(-1);
+ number -= BigInt(1);
+ }
+ if (scale) {
Review Comment:
```suggestion
if (typeof scale === 'number') {
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1534940833
##########
js/src/util/bn.ts:
##########
@@ -68,25 +68,36 @@ Object.assign(SignedBigNum.prototype, BigNum.prototype, { 'constructor': SignedB
Object.assign(UnsignedBigNum.prototype, BigNum.prototype, { 'constructor': UnsignedBigNum, 'signed': false, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
Object.assign(DecimalBigNum.prototype, BigNum.prototype, { 'constructor': DecimalBigNum, 'signed': true, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
+//FOR ES2020 COMPATIBILITY
+const TWO_TO_THE_64 = BigInt(4294967296) * BigInt(4294967296); // 2^32 * 2^32 = 0x10000000000000000n
+const TWO_TO_THE_64_MINUS_1 = TWO_TO_THE_64 - BigInt(1); // (2^32 * 2^32) - 1 = 0xFFFFFFFFFFFFFFFFn
+
/** @ignore */
-function bigNumToNumber<T extends BN<BigNumArray>>(bn: T) {
- const { buffer, byteOffset, length, 'signed': signed } = bn;
- const words = new BigUint64Array(buffer, byteOffset, length);
+export const bigNumToNumber: { <T extends BN<BigNumArray>>(bn: T, denominator?: bigint): number } = (<T extends BN<BigNumArray>>(bn: T, denominator?: bigint) => {
Review Comment:
Let's leave this as a function. I think the "functions" below also should be functions and not consts with an arrow function.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2014197628
`vec.get(0).valueOf(1000n)` return the correct value but it would be nice to pass the scale though. It's in the type but I don't know what is the best way to get that. This otherwise looks correct.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015885789
It wouldn't build, we had the same issue on our older code (pre es2020) -
think ** and bigint support is no bueno for older builds
On Fri, Mar 22, 2024 at 1:45 PM Dominik Moritz ***@***.***>
wrote:
> Ah your suggestion broke it
> <https://github.com/apache/arrow/pull/40729/commits/cff7709d63f03f497dbdc3e97a6fcbd16a1d2f62>
> so I reverted.
>
> What did it break?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#issuecomment-2015880468>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGH5XXQHASPVBEOS3JTYZSJ6TAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVHA4DANBWHA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz merged PR #40729:
URL: https://github.com/apache/arrow/pull/40729
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2014266652
Thanks for looking at the tests.
I see. How do you imagine someone using get then? We should probably do the correct conversion there. Maybe we can cache the scale to denominator conversion somewhere if you're worried about performance.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1536817745
##########
js/test/unit/bn-tests.ts:
##########
@@ -83,4 +83,19 @@ describe(`BN`, () => {
const d4 = toDecimal(new Uint32Array([0x9D91E773, 0x4BB90CED, 0xAB2354CC, 0x54278E9B]));
expect(d4.toString()).toBe('111860543658909349380118287427608635251');
});
+
+ test(`valueOf for decimal numbers`, () => {
+ const n1 = new BN(new Uint32Array([0x00000001, 0x00000000, 0x00000000, 0x00000000]), false);
+ expect(n1.valueOf()).toBe(1);
+ const n2 = new BN(new Uint32Array([0xFFFFFFFE, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n2.valueOf()).toBe(-2);
+ const n3 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n3.valueOf()).toBe(-1);
+ const n4 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n4.valueOf(1)).toBe(-0.1);
+ const n5 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ expect(n5.valueOf()).toBe(1.7014118346046923e+38);
+ // const n6 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ // expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
Review Comment:
Why is this commented out?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2017155143
Because the CI tool expects all transformations to be reversible and
converting from a decimal to a number is a one way operation - so I had to
comment it out.
On Sun, Mar 24, 2024 at 06:34 Dominik Moritz ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In js/test/unit/bn-tests.ts
> <https://github.com/apache/arrow/pull/40729#discussion_r1536817745>:
>
> > + // const n6 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
> + // expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
>
> Why is this commented out?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#pullrequestreview-1956554607>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGFHVMSFXAVZMXMYUYLYZ3I6TAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJWGU2TINRQG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2018563889
Honestly doing anything is an improvement on where the code is - so please just do what you'd like.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015882019
Also, please make a new issue that this pull request refers to since it doesn't address the referenced issue fully yet.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2018538175
Hey @domoritz looks like you introduced another breaking change, while testing for number may seem ok, it will break for NaN, and further slow down the method.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1534954046
##########
js/src/util/bn.ts:
##########
@@ -68,25 +68,36 @@ Object.assign(SignedBigNum.prototype, BigNum.prototype, { 'constructor': SignedB
Object.assign(UnsignedBigNum.prototype, BigNum.prototype, { 'constructor': UnsignedBigNum, 'signed': false, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
Object.assign(DecimalBigNum.prototype, BigNum.prototype, { 'constructor': DecimalBigNum, 'signed': true, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
+//FOR ES2020 COMPATIBILITY
+const TWO_TO_THE_64 = BigInt(4294967296) * BigInt(4294967296); // 2^32 * 2^32 = 0x10000000000000000n
Review Comment:
Why not just this?
```suggestion
const TWO_TO_THE_64 = BigInt(2) ** BigInt(64); // 2^64 = 0x10000000000000000n
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015106723
I think the best way forward is to get this pull request in but make a new issue that this pull request is attached to. We probably need to refactor the whole decimal handling in ways that are orthogonal to this fix. How does that sound @dioptre?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015521761
Updated the PR, tests look ok?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015668515
Ah your [suggestion broke it](https://github.com/apache/arrow/pull/40729/commits/cff7709d63f03f497dbdc3e97a6fcbd16a1d2f62) so I reverted. I also made it a function per your request.
It was my understanding that the scale is included in the metadata, and there's no real metadata in the arrow bytes. This confused me at first as I was looking for it. So seems we need the valueOf(scale)?
> console.log(vec.get(0)) // [1000, 0, 0, 0] <- byte array looks correct
> console.log(`${vec.get(0)}`) // "1000" <- incorrect, should be 1; ignores scale
> console.log(Number(vec.get(0))) // 1000 <- incorrect, should be 1
> ```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015306205
I was thinking the same. Perhaps we go for the scale first if you want to
keep it in that format for the future and we can optimize later.
If you are happy with that I can change denominator to scale and we can
look at cache later.
On Fri, Mar 22, 2024 at 06:32 Dominik Moritz ***@***.***>
wrote:
> I think the best way forward is to get this pull request in but make a new
> issue that this pull request is attached to. We probably need to refactor
> the whole decimal handling in ways that are orthogonal to this fix. How
> does that sound @dioptre <https://github.com/dioptre>?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#issuecomment-2015106723>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGCLRZM7FC46J25DUFLYZQXFVAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVGEYDMNZSGM>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2016583792
Sounds good. Let me know if there’s anything else I can help with.
On Sat, Mar 23, 2024 at 05:53 Dominik Moritz ***@***.***>
wrote:
> Fair. We're need to refactor how all of this is used for decimals a bit
> anyway so lets leave it.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/40729#issuecomment-2016486277>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFZTGCM4BLWM6XSGVLWT6LYZV3NJAVCNFSM6AAAAABFCPNKICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJWGQ4DMMRXG4>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2018702471
Yep. let's get this in and improve things in follow up PRs. I think we should introduce a new class so we can automatically apply the right scale from the type.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015968592
Think should be good to run again
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1537964059
##########
js/test/unit/bn-tests.ts:
##########
@@ -83,4 +83,19 @@ describe(`BN`, () => {
const d4 = toDecimal(new Uint32Array([0x9D91E773, 0x4BB90CED, 0xAB2354CC, 0x54278E9B]));
expect(d4.toString()).toBe('111860543658909349380118287427608635251');
});
+
+ test(`valueOf for decimal numbers`, () => {
+ const n1 = new BN(new Uint32Array([0x00000001, 0x00000000, 0x00000000, 0x00000000]), false);
+ expect(n1.valueOf()).toBe(1);
+ const n2 = new BN(new Uint32Array([0xFFFFFFFE, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n2.valueOf()).toBe(-2);
+ const n3 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n3.valueOf()).toBe(-1);
+ const n4 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n4.valueOf(1)).toBe(-0.1);
+ const n5 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ expect(n5.valueOf()).toBe(1.7014118346046923e+38);
+ // const n6 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ // expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
Review Comment:
> I don't understand. What does reversible mean here? Will this ever work or should we just delete it?
The CI tool does not allow transformations from BigInt to number as data (correctly) will be lost. What the CI tool is not smart enough to realize is, this is on purpose.
It's important that people still realize this is still possible and testable, so removing it from the test is not as good an option as leaving this commented out in the test.
I think considering we have a very broken library at present, this should probably not be of consequence. So whatever you prefer. My preference is to get the fix in and we can talk about this later.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2020559297
@dioptre there seems to be an issue with fractional scales
```
const n5 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
expect(n5.valueOf(0.1)).toBe(1.7014118346046923e+38);
```
Which leads to `RangeError: The number 1.2589254117941673 cannot be converted to a BigInt because it is not an integer`.
This is supposed to work since decimals can be fractions, right?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2019423105
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 8efb8dce97693289e7a2dd780b6385714f07a5e3.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/23088418691) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015880468
> Ah your [suggestion broke it](https://github.com/apache/arrow/pull/40729/commits/cff7709d63f03f497dbdc3e97a6fcbd16a1d2f62) so I reverted.
What did it break?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1537920807
##########
js/test/unit/bn-tests.ts:
##########
@@ -83,4 +83,19 @@ describe(`BN`, () => {
const d4 = toDecimal(new Uint32Array([0x9D91E773, 0x4BB90CED, 0xAB2354CC, 0x54278E9B]));
expect(d4.toString()).toBe('111860543658909349380118287427608635251');
});
+
+ test(`valueOf for decimal numbers`, () => {
+ const n1 = new BN(new Uint32Array([0x00000001, 0x00000000, 0x00000000, 0x00000000]), false);
+ expect(n1.valueOf()).toBe(1);
+ const n2 = new BN(new Uint32Array([0xFFFFFFFE, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n2.valueOf()).toBe(-2);
+ const n3 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n3.valueOf()).toBe(-1);
+ const n4 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
+ expect(n4.valueOf(1)).toBe(-0.1);
+ const n5 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ expect(n5.valueOf()).toBe(1.7014118346046923e+38);
+ // const n6 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
+ // expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
Review Comment:
> Because the CI tool expects all transformations to be reversible and
converting from a decimal to a number is a one way operation - so I had to
comment it out.
I don't understand. What does reversible mean here? Will this ever work or should we just delete it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37920: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015499458
Let's change to scale and make a new issue. Then we can merge.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2021354217
We can get the scale from the type. I misunderstood how the scale works. Thanks for the link that explained what I was missing.
```
const n3 = new BN(new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF]), true);
expect(n3.valueOf()).toBe(-1);
expect(n3.valueOf(1)).toBe(-0.1);
expect(n3.valueOf(2)).toBe(-0.01);
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2015954175
I fixed the title of this pull request. Now we just need the integration tests to pass.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2016486277
Fair. We're need to refactor how all of this is used for decimals a bit anyway so lets leave it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2018707395
Thank you
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on code in PR #40729:
URL: https://github.com/apache/arrow/pull/40729#discussion_r1537957001
##########
js/src/util/bn.ts:
##########
@@ -68,24 +68,36 @@ Object.assign(SignedBigNum.prototype, BigNum.prototype, { 'constructor': SignedB
Object.assign(UnsignedBigNum.prototype, BigNum.prototype, { 'constructor': UnsignedBigNum, 'signed': false, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
Object.assign(DecimalBigNum.prototype, BigNum.prototype, { 'constructor': DecimalBigNum, 'signed': true, 'TypedArray': Uint32Array, 'BigIntArray': BigUint64Array });
+//FOR ES2020 COMPATIBILITY
+const TWO_TO_THE_64 = BigInt(4294967296) * BigInt(4294967296); // 2^64 = 0x10000000000000000n
+const TWO_TO_THE_64_MINUS_1 = TWO_TO_THE_64 - BigInt(1); // (2^32 * 2^32) - 1 = 0xFFFFFFFFFFFFFFFFn
+
/** @ignore */
-function bigNumToNumber<T extends BN<BigNumArray>>(bn: T) {
- const { buffer, byteOffset, length, 'signed': signed } = bn;
- const words = new BigUint64Array(buffer, byteOffset, length);
+export function bigNumToNumber<T extends BN<BigNumArray>>(bn: T, scale?: number) {
+ const { buffer, byteOffset, byteLength, 'signed': signed } = bn;
+ const words = new BigUint64Array(buffer, byteOffset, byteLength / 8);
const negative = signed && words.at(-1)! & (BigInt(1) << BigInt(63));
- let number = negative ? BigInt(1) : BigInt(0);
- let i = BigInt(0);
+ let number = BigInt(0);
+ let i = 0;
if (!negative) {
for (const word of words) {
- number += word * (BigInt(1) << (BigInt(32) * i++));
+ number |= word * (BigInt(1) << BigInt(64 * i++));
}
} else {
for (const word of words) {
- number += ~word * (BigInt(1) << (BigInt(32) * i++));
+ number |= (word ^ TWO_TO_THE_64_MINUS_1) * (BigInt(1) << BigInt(64 * i++));
}
number *= BigInt(-1);
+ number -= BigInt(1);
+ }
+ if (scale) {
Review Comment:
This is going to further slow down execution, also it won't account for NaN.
After all this I'd like to bring something back (maybe an additional method) that is efficient after getting this change in. Think good enough it replaces the broken method for now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40755: [JS] fix decimal conversions [arrow]
Posted by "dioptre (via GitHub)" <gi...@apache.org>.
dioptre commented on PR #40729:
URL: https://github.com/apache/arrow/pull/40729#issuecomment-2021126172
@domoritz as far as I can tell the decimal binary storage stores only an integer.
The arrow metadata / schema dictates the scale and precision. These are integer values per this description: https://www.logicbig.com/quick-info/programming/precision-and-scale.html
I haven't read anything about arrow storing any metadata in the hex like most other decimal systems do, and it seems to come through only the parent metadata / schema.
Combining the schema, and the BN (Arrow's BigNum) we get the actual number.
The scale must be an integer per the link above. So the test you shared is invalid I believe.
We ended up going with scale in the valueOf function after your feedback instead of the denominator as I originally suggested. This requires us to ```10^scale``` for each function result. It's obviously a bit slower but hopefully makes more sense for users.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org