You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org> on 2026/02/09 15:47:41 UTC
[PR] Gpatterson/issue 15 interval mdn tests [arrow-js]
GeorgeLeePatterson opened a new pull request, #379:
URL: https://github.com/apache/arrow-js/pull/379
## Summary
- fix "'`IntervalMonthDayNano` conversion to avoid `BigInt(number)` precision traps for unsafe integers
- add regression coverage in `test/unit/vector/interval-month-day-nano-tests.ts` for known failing values from issue #15
- add `parseArrowJSON()` (`src/util/json.ts`) to normalize unsafe integer `number` values from `json-bignum` into `BigNumber`
- use `parseArrowJSON()` in JSON ingestion scripts:
- `bin/json-to-arrow.ts`
- `bin/integration.ts`
- `src/bin/arrow2csv.ts`
## Rationale
`json-bignum` can yield unsafe integers as `number` for values whose decimal text round-trips through `Number#toString()`. Passing those numbers directly into `BigInt(...)` can introduce nanosecond deltas in `IntervalMonthDayNano` integration comparisons.
This PR removes that loss path in two places:
1. conversion edge (`toIntervalMonthDayNanoInt32Array`)
2. JSON parser boundary normalization (`parseArrowJSON`)
## Testing
- `yarn eslint src/util/json.ts src/util/interval.ts src/bin/arrow2csv.ts test/unit/vector/interval-month-day-nano-tests.ts`
- `yarn jest test/unit/vector/interval-month-day-nano-tests.ts --runInBand`
- `yarn jest test/unit/vector --runInBand`
## Notes
- Integration tests were not run locally.
Ref: #15
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3885447967
> The changes look good to me now @GeorgeLeePatterson . Can you just fix the PR description to match the current PR contents?
Done, let me know if it looks good.
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2792656814
##########
src/util/interval.ts:
##########
@@ -46,7 +46,11 @@ export function toIntervalMonthDayNanoInt32Array(objects: Partial<IntervalMonthD
data[ai++] = interval['days'] ?? 0;
const nanoseconds = interval['nanoseconds'];
if (nanoseconds) {
- const ns = BigInt(nanoseconds);
+ // Avoid BigInt(number) for unsafe integers, which can embed
+ // IEEE-754 rounding before conversion.
Review Comment:
Agreed. I'll update 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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3884518651
@trxcllnt I'm not a JS/TS developer so I would appreciate if you could give this a quick look and validate the code in this PR.
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2794596906
##########
src/util/json.ts:
##########
@@ -0,0 +1,28 @@
+// 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.
+
+// @ts-ignore
+import jsonBigInt from 'json-bigint';
+
+const parseJSON = jsonBigInt({ useNativeBigInt: true }).parse;
Review Comment:
Should we use `useNativeBigInt: true` here? From the [Note on native BigInt support](https://www.npmjs.com/package/json-bigint#note-on-native-bigint-support) section of the readme, it seems BigInts are converted to JS Numbers, which doesn't seem correct?
From the description it seems we might want to set `storeAsString: true` instead, since we're more concerned about accuracy and not inferring the value type based on the representation in the data. For example, we know the values in the IntervalMonthDayYear `DATA` array are 64-bit signed integers, regardless whether they're stored as strings in the JSON. Thoughts?
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2783770345
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,39 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds parsed as numbers roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = Number.parseFloat(sample);
Review Comment:
This seems incorrect since that will introduce FP inaccuracies. You need something like `BigInt(sample)`.
For example `console.log(BigInt(Number.parseFloat("-390122861233460600")));` gives me `-390122861233460608n`.
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2792699815
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,42 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds represented as bigint roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = BigInt(sample);
+ const obj: Partial<IntervalMonthDayNanoObject> = { nanoseconds };
Review Comment:
I see, thanks.
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2792660568
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,42 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds represented as bigint roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = BigInt(sample);
+ const obj: Partial<IntervalMonthDayNanoObject> = { nanoseconds };
Review Comment:
Has the same effect when the key name and variable name are the same.
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2800700835
##########
src/util/json.ts:
##########
@@ -0,0 +1,28 @@
+// 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.
+
+// @ts-ignore
+import jsonBigInt from 'json-bigint';
+
+const parseJSON = jsonBigInt({ useNativeBigInt: true }).parse;
Review Comment:
This is a good point, first a convention based approach will be hard to catch, as you identified. That should be addressed, but ultimately will come down to some level of convention at some point. But that other issue really is the more concerning one. What's the standard way of handling this, I assume vendor-ing their code isn't preferable, but maybe that's the only option?
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3904833972
@trxcllnt Library changed, updated the test with some additional numbers derived from previous failures, but otherwise preserved what was there. Let me know if anything needs additional tweaking.
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2783770345
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,39 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds parsed as numbers roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = Number.parseFloat(sample);
Review Comment:
This seems incorrect since that will introduce FP inaccuracies. You need `BigInt(sample)`.
For example `console.log(BigInt(Number.parseFloat("-390122861233460600")));` gives me `-390122861233460608n`.
##########
src/util/json.ts:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+// @ts-ignore
+import { parse as bignumJSONParse, BigNumber } from 'json-bignum';
+
+/** @ignore */
+export function parseArrowJSON(source: string): any {
+ return normalizeUnsafeIntegers(bignumJSONParse(source));
+}
+
+function normalizeUnsafeIntegers(value: any): any {
+ if (typeof value === 'number') {
+ if (Number.isInteger(value) && !Number.isSafeInteger(value)) {
Review Comment:
Oops, I see that library is unmaintained. If there is a better replacement, perhaps we should just switch to that?
##########
src/util/json.ts:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+// @ts-ignore
+import { parse as bignumJSONParse, BigNumber } from 'json-bignum';
+
+/** @ignore */
+export function parseArrowJSON(source: string): any {
+ return normalizeUnsafeIntegers(bignumJSONParse(source));
+}
+
+function normalizeUnsafeIntegers(value: any): any {
+ if (typeof value === 'number') {
+ if (Number.isInteger(value) && !Number.isSafeInteger(value)) {
Review Comment:
So this would be for values such as `100000000000000000`? Should this be reported as a bug in `json-bignum`?
##########
src/util/json.ts:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+// @ts-ignore
+import { parse as bignumJSONParse, BigNumber } from 'json-bignum';
+
+/** @ignore */
+export function parseArrowJSON(source: string): any {
+ return normalizeUnsafeIntegers(bignumJSONParse(source));
+}
+
+function normalizeUnsafeIntegers(value: any): any {
+ if (typeof value === 'number') {
+ if (Number.isInteger(value) && !Number.isSafeInteger(value)) {
Review Comment:
Ok, it seems https://www.npmjs.com/package/json-bigint calls `Number.isSafeInteger` correctly (and also can produce native JS `BigInt` instead of a home-grown type).
##########
src/util/json.ts:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+// @ts-ignore
+import { parse as bignumJSONParse, BigNumber } from 'json-bignum';
+
+/** @ignore */
+export function parseArrowJSON(source: string): any {
+ return normalizeUnsafeIntegers(bignumJSONParse(source));
+}
+
+function normalizeUnsafeIntegers(value: any): any {
Review Comment:
Can you add a comment explaining the rationale as you did in the PR description?
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,39 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds parsed as numbers roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = Number.parseFloat(sample);
+ expect(`${nanoseconds}`).toBe(sample);
+ const obj: Partial<IntervalMonthDayNanoObject> = { nanoseconds };
+ const array = toIntervalMonthDayNanoInt32Array([obj]);
+ const vec = makeIntervalMonthDayNanoVector(array);
+ expect(vec.type).toBeInstanceOf(IntervalMonthDayNano);
Review Comment:
Should we also call `expect(vec.get(0)).toStrictEqual(array);` as above?
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3897786864
@pitrou @trxcllnt Did we land on a final decision?
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2792656814
##########
src/util/interval.ts:
##########
@@ -46,7 +46,11 @@ export function toIntervalMonthDayNanoInt32Array(objects: Partial<IntervalMonthD
data[ai++] = interval['days'] ?? 0;
const nanoseconds = interval['nanoseconds'];
if (nanoseconds) {
- const ns = BigInt(nanoseconds);
+ // Avoid BigInt(number) for unsafe integers, which can embed
+ // IEEE-754 rounding before conversion.
Review Comment:
Agreed. I'll update 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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2800638747
##########
src/util/json.ts:
##########
@@ -0,0 +1,28 @@
+// 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.
+
+// @ts-ignore
+import jsonBigInt from 'json-bigint';
+
+const parseJSON = jsonBigInt({ useNativeBigInt: true }).parse;
Review Comment:
The serialization part makes sense, but I was concerned about this:
```js
o !== JSONbig.parse(JSONbig.stringify(o))
```
It seems like if parse sometimes produces BitInt and other times Number, we'd have to know to always cast the numbers to BigInt after parsing and hope that the values that parsed as numbers didn't lose precision.
That said, I think [this](https://github.com/sidorares/json-bigint/issues/88#issuecomment-1935464056) is a bigger issue. It sounds like we can't `JSONbig.parse()` any payloads that includes decimals, which seems problematic if parsing an ArrowJSON message with both a Float64 and IntervalMonthDayYear column.
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3892985463
> That said, https://www.npmjs.com/package/json-with-bigint looks nice and adequate for our purposes.
Interesting, I'd have to compare the core functionality to identify if it in fact has parity with what we need, but if it does, their JS is only 139 lines. At that point I'd almost say vendoring is the best option.
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2793215885
##########
src/util/interval.ts:
##########
@@ -46,7 +46,11 @@ export function toIntervalMonthDayNanoInt32Array(objects: Partial<IntervalMonthD
data[ai++] = interval['days'] ?? 0;
const nanoseconds = interval['nanoseconds'];
if (nanoseconds) {
- const ns = BigInt(nanoseconds);
+ // Avoid BigInt(number) for unsafe integers, which can embed
+ // IEEE-754 rounding before conversion.
Review Comment:
Had a chance to review, and you are correct. I will fix and update shortly.
--
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] fix: interval MonthDayNano precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3920139038
> @GeorgeLeePatterson Could you update the PR description before we merge this?
Done. Let me know if it suffices.
--
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] fix: interval MonthDayNano precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3918114035
@GeorgeLeePatterson Could you update the PR description before we merge this?
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3900358016
Yeah, can you test it with `json-with-bigint` instead of `json-bigint`?
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2784237460
##########
src/util/json.ts:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+// @ts-ignore
+import { parse as bignumJSONParse, BigNumber } from 'json-bignum';
+
+/** @ignore */
+export function parseArrowJSON(source: string): any {
+ return normalizeUnsafeIntegers(bignumJSONParse(source));
+}
+
+function normalizeUnsafeIntegers(value: any): any {
+ if (typeof value === 'number') {
+ if (Number.isInteger(value) && !Number.isSafeInteger(value)) {
Review Comment:
Good catch
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3885939514
I just looked at the `json-bigint` commit history and issues. Seems like it may be similarly un-maintained? [This issue](https://github.com/sidorares/json-bigint/issues/99) mentions alternatives.
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3887251253
> I just looked at the `json-bigint` commit history and issues. Seems like it may be similarly un-maintained? [This issue](https://github.com/sidorares/json-bigint/issues/99) mentions alternatives.
Ahah, the joys of the JavaScript ecosystem...
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2784239375
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,39 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds parsed as numbers roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = Number.parseFloat(sample);
Review Comment:
Another good catch. I'll get these shored up.
--
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] fix: issue 15 interval mdn tests [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2792172430
##########
test/unit/vector/interval-month-day-nano-tests.ts:
##########
@@ -77,4 +78,42 @@ describe(`MonthDayNanoIntervalVector`, () => {
expect(vec.get(0)).toStrictEqual(array);
expect(toIntervalMonthDayNanoObjects(vec.get(0), false)).toStrictEqual([{ ...EMPTY_INTERVAL_MONTH_DAY_NANO_OBJECT, ...obj }]);
});
+
+ test(`Unsafe integer nanoseconds represented as bigint roundtrip correctly`, () => {
+ const samples = [
+ '-390122861233460600',
+ '6684525287992311000'
+ ];
+ for (const sample of samples) {
+ const nanoseconds = BigInt(sample);
+ const obj: Partial<IntervalMonthDayNanoObject> = { nanoseconds };
Review Comment:
Is the right field being populated here, or should that be `{ nanoseconds : nanoseconds }`? Sorry, not a TS developer.
##########
src/util/interval.ts:
##########
@@ -46,7 +46,11 @@ export function toIntervalMonthDayNanoInt32Array(objects: Partial<IntervalMonthD
data[ai++] = interval['days'] ?? 0;
const nanoseconds = interval['nanoseconds'];
if (nanoseconds) {
- const ns = BigInt(nanoseconds);
+ // Avoid BigInt(number) for unsafe integers, which can embed
+ // IEEE-754 rounding before conversion.
Review Comment:
Is this still any useful? I don't understand why ```BigInt(`${nanoseconds}`)``` would be any better than `BigInt(nanoseconds)`. If the data is already rounded, we cannot restore the lost precision...
What we could do is call `nanoseconds.isSafeInteger` and warn/assert if it returns false?
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2794596906
##########
src/util/json.ts:
##########
@@ -0,0 +1,28 @@
+// 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.
+
+// @ts-ignore
+import jsonBigInt from 'json-bigint';
+
+const parseJSON = jsonBigInt({ useNativeBigInt: true }).parse;
Review Comment:
Should we use `useNativeBigInt: true` here? From the [Note on native BigInt support](https://www.npmjs.com/package/json-bigint#note-on-native-bigint-support) section of the readme, it seems BigInts are converted to JS Numbers, which doesn't seem correct?
From the description it seems we might want to set `storeAsString: true` instead, since we're more concerned about accuracy, not trying to infer the value type based on the representation in the data. For example, we know the values in the IntervalMonthDayYear `DATA` array are 64-bit signed integers, regardless whether they're stored as strings in the JSON. Thoughts?
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3880416589
@pitrou I think we're ready for another go at it. Here's a quick synopsis:
* switched parser to json-bigint
* removed unsafe-number test path
* fixed UMD-incompatible test import
* local matrix passed
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3887257872
That said, https://www.npmjs.com/package/json-with-bigint looks nice and adequate for our purposes.
--
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] fix: interval MonthDayNano precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3920101235
Sure, let me note the change in library.
--
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] Gpatterson/issue 15 interval mdn tests [arrow-js]
Posted by "GeorgeLeePatterson (via GitHub)" <gi...@apache.org>.
GeorgeLeePatterson commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3873515542
Oh yes, I see what you mean 🤦♂️ I carried those over from another branch I had working on this that I hadn't visited in a while, and neglected to double check them. Let me get those fixed up.
I completely agree, I thought the same but worked with the constraint I had to use the same lib. But if we can improve that aspect I think it would help stabilize a lot.
--
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] fix #15: interval mdn precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #379:
URL: https://github.com/apache/arrow-js/pull/379#discussion_r2795551423
##########
src/util/json.ts:
##########
@@ -0,0 +1,28 @@
+// 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.
+
+// @ts-ignore
+import jsonBigInt from 'json-bigint';
+
+const parseJSON = jsonBigInt({ useNativeBigInt: true }).parse;
Review Comment:
> From the [Note on native BigInt support](https://www.npmjs.com/package/json-bigint#note-on-native-bigint-support) section of the readme, it seems BigInts are converted to JS Numbers, which doesn't seem correct?
I read this as: on the serialization path, a JS BigInt is serialized as a [JSON number](https://datatracker.ietf.org/doc/html/rfc8259#section-6).
[The example](https://www.npmjs.com/package/json-bigint#optionsusenativebigint-boolean-default-false) shows that parsing doesn't lose precision.
--
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] fix: interval MonthDayNano precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #379:
URL: https://github.com/apache/arrow-js/pull/379#issuecomment-3924974313
Thanks. I'll merge this.
--
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] fix: interval MonthDayNano precision loss when reading JSON-encoded int64s [arrow-js]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #379:
URL: https://github.com/apache/arrow-js/pull/379
--
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