Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-5714: [JS] Inconsistent behavior in Int64Builder with/without BigNum #4691

Closed

Conversation

TheNeuralBit
Copy link
Member

@TheNeuralBit TheNeuralBit commented Jun 25, 2019

  • Use stride in DataBufferBuilder.set to ensure that Int64Builder is consistent with and without BigNum.
  • Use WideBufferBuilder for Int64 and Uint64 builders, even when BigInt is not available. Moves check for BigInt availability into WideBufferBuilder. (Thanks to Paul Taylor)

@trxcllnt
Copy link
Contributor

trxcllnt commented Jun 25, 2019

@TheNeuralBit I was thinking about this more last night, and while this works for the JS-number case, it won't work if we try to set in a pair of [lo,hi]. So my thought was we should refactor the WideBufferBuilder so we could use it for the 64bit types, and just provide the BigInt TypedArray views if they're available.

@trxcllnt
Copy link
Contributor

@TheNeuralBit TheNeuralBit#3 here's a pull that should do what we want in FF

@codecov-io
Copy link

Codecov Report

Merging #4691 into master will increase coverage by 1.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4691      +/-   ##
=========================================
+ Coverage   88.94%   90.2%   +1.25%     
=========================================
  Files         705     101     -604     
  Lines       81275    6399   -74876     
  Branches     1420    1420              
=========================================
- Hits        72288    5772   -66516     
+ Misses       8977     617    -8360     
  Partials       10      10
Impacted Files Coverage Δ
js/src/builder/buffer.ts 91.48% <100%> (ø) ⬆️
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
r/src/symbols.cpp
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/parquet/column_scanner.cc
r/R/ArrayData.R
... and 595 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 0568544...3688820. Read the comment docs.

@TheNeuralBit TheNeuralBit force-pushed the js-data-buffer-builder-stride branch from 3688820 to 2e4ce7a Compare June 26, 2019 14:44
Use WideBufferBuilder for Int64 and Uint64 builders
* Also modifies logic slightly, now the various input types are evenly
distributed.
@TheNeuralBit
Copy link
Member Author

@trxcllnt I merged your PR and also clarified the int64 generation for the builder tests. Does this look ok to you?

@trxcllnt
Copy link
Contributor

@TheNeuralBit yep, that looks great. thanks!

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.

3 participants