Skip to content

Conversation

@trxcllnt
Copy link
Contributor

This started as a continuation of #3634, but grew enough to deserve its own PR. I've made a PR to my own fork that highlights just the changes here: trxcllnt#8. I'll rebase this PR after #3634 is merged so only these changes are included.

This PR reverts the behavior of Float16Vector#toArray() back to returning a zero-copy slice of the underlying Uint16Array data, and exposes the copying behavior via new toFloat32Array() and toFloat64Array() methods. Float16Array.from() will also convert any incoming 32 or 64-bit floats to Uint16s if necessary.

It also adds tighter integration with the new BigInt, BigInt64Array, and BigUint64Array primitives (if available):

  1. Use the native BigInt to convert/stringify i64s/u64s
  2. Support the BigInt type in element comparator and indexOf()
  3. Add zero-copy toBigInt64Array() and toBigUint64Array() methods to Int64Vector and Uint64Vector, respectively

0.4.0 added support for basic conversion to the native BigInt when available, but would only create positive BigInts, and was slower than necessary. This PR uses the native Arrays to create the BigInts, so we should see some speed ups there. Ex:

const vec = Int64Vector.from(new Int32Array([-1, 2147483647]))
const big = vec.get(0)
assert(big[0] === -1) // true
assert(big[1] === 2147483647) // true
const num = 0n + big // or BigInt(big)
assert(num === (2n ** 63n - 1n)) // true

JIRAs associated with this PR are:

  • ARROW-4578 - Float16Vector toArray should be zero-copy
  • ARROW-4579 - Add more interop with BigInt/BigInt64Array/BigUint64Array
  • ARROW-4580 - Accept Iterables in IntVector/FloatVector from() signatures

@codecov-io
Copy link

codecov-io commented Feb 14, 2019

Codecov Report

Merging #3653 into master will increase coverage by 2.85%.
The diff coverage is 88.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3653      +/-   ##
==========================================
+ Coverage   87.47%   90.33%   +2.85%     
==========================================
  Files         627       74     -553     
  Lines       80413     5413   -75000     
  Branches        0     1225    +1225     
==========================================
- Hits        70344     4890   -65454     
+ Misses       9960      515    -9445     
+ Partials      109        8     -101
Impacted Files Coverage Δ
js/src/visitor/toarray.ts 98.21% <100%> (ø)
js/src/util/vector.ts 95.69% <100%> (ø)
js/src/util/bit.ts 100% <100%> (ø)
js/src/visitor/jsonvectorassembler.ts 100% <100%> (ø)
js/src/util/compat.ts 66.66% <60.86%> (ø)
js/src/util/bn.ts 72.22% <68.18%> (ø)
js/src/vector/float.ts 91.66% <86.66%> (ø)
js/src/vector/int.ts 90.9% <87.87%> (ø)
js/src/util/buffer.ts 93.7% <90%> (ø)
js/src/type.ts 93.72% <98.91%> (ø)
... and 701 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1442fb6...69ee6f7. Read the comment docs.

@domoritz
Copy link
Member

This is great. Native BigInt support will get better and better and so having good support for it in Arrow is great.

@trxcllnt trxcllnt force-pushed the js/int-and-float-fixes branch from c9613b2 to 7b96b5f Compare February 18, 2019 02:31
@trxcllnt trxcllnt force-pushed the js/int-and-float-fixes branch 2 times, most recently from 6350e9e to 2557277 Compare February 21, 2019 07:28
@TheNeuralBit
Copy link
Member

👍 I also appreciate you looking out for firefox users and handling the case where BigInt is not supported

Can you rebase now that #3634 is merged?

@trxcllnt trxcllnt force-pushed the js/int-and-float-fixes branch from 2557277 to 69ee6f7 Compare February 21, 2019 17:51
@trxcllnt
Copy link
Contributor Author

@TheNeuralBit ok rebase is done. everything looks alright to me, let me know if you notice anything weird though.

Copy link
Member

@TheNeuralBit TheNeuralBit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants