fix: interval MonthDayNano precision loss when reading JSON-encoded int64s

[GeorgeLeePatterson]

Summary

This PR fixes precision handling for IntervalMonthDayNano nanoseconds when reading JSON, and aligns the tests with the actual behavior across build targets.

Changes

• Switched JSON parsing in src/util/json.ts from json-bignum to json-with-bigint with native BigInt output.
• Removed post-parse unsafe-integer normalization logic that could not recover precision once rounded.
• Updated test/unit/vector/interval-month-day-nano-tests.ts to:
  • assert parsed nanoseconds are bigint,
  • include the additional expect(vec.get(0)).toStrictEqual(array) assertion.

Validation

Ran locally:

• yarn lint:ci
• yarn test

Closes#15.

[pitrou]

Thanks for looking into this @GeorgeLeePatterson !

It seems the tests are incorrect. Furthermore, I think we can largely simplify this by switching to a better JSON parsing library, see comments below.

[GeorgeLeePatterson]

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.

[GeorgeLeePatterson]

@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

[pitrou]

This looks much better, thank you. Two comments still.

[pitrou]

The changes look good to me now @GeorgeLeePatterson . Can you just fix the PR description to match the current PR contents?

[pitrou]

@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.

[GeorgeLeePatterson]

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.

[trxcllnt]

Thanks for the PR, looks like @pitrou already addressed most of the questions I had.

I agree with the switch from json-bignum to json-bigint, just left one comment about usage that I'd like to discuss before approving.

[trxcllnt]

Should we use useNativeBigInt: true here? From the 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?

[pitrou]

From the 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.

The example shows that parsing doesn't lose precision.

[trxcllnt]

The serialization part makes sense, but I was concerned about this:

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 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.

[GeorgeLeePatterson]

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?

[trxcllnt]

I just looked at the json-bigint commit history and issues. Seems like it may be similarly un-maintained? This issue mentions alternatives.

[pitrou]

I just looked at the json-bigint commit history and issues. Seems like it may be similarly un-maintained? This issue mentions alternatives.

Ahah, the joys of the JavaScript ecosystem...

[pitrou]

That said, https://www.npmjs.com/package/json-with-bigint looks nice and adequate for our purposes.

[GeorgeLeePatterson]

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.

[GeorgeLeePatterson]

@pitrou@trxcllnt Did we land on a final decision?

[trxcllnt]

Yeah, can you test it with json-with-bigint instead of json-bigint?

[GeorgeLeePatterson]

@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.

[trxcllnt]

Looks good to me, thanks @GeorgeLeePatterson!

[kou]

@GeorgeLeePatterson Could you update the PR description before we merge this?

[GeorgeLeePatterson]

Sure, let me note the change in library.

[GeorgeLeePatterson]

@GeorgeLeePatterson Could you update the PR description before we merge this?

Done. Let me know if it suffices.

[kou]

Thanks. I'll merge this.

[alamb]

Thank you @GeorgeLeePatterson -- I am pretty excited to see this fixed (we keep hitting it intermittently in arrow-rs)