-
Notifications
You must be signed in to change notification settings - Fork 9
Use const 8 byteWidth for Timestamp and Duration #130
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
Conversation
I realized that the only places calling getTimeByteWidth were the Timestamp and Duration blocks, which are the two blocks that always use BigInt64Array. So rather than keep the function call for those two scenarios, which should (if I understand correctly) always return 8 anyway, I just dropped in the constant.
|
It would be great to have tests specifically for the time, duration, and timestamp array types. I think you just need to update the version of |
|
It looks like #131 at least gets past that error point |
|
You can try merging in main and see if that fixes your tests |
I wasn't sure if I should commit these or not, but this is what I had to do in onder to get yarn test to run locally.
|
I merged in main, and went through and got I see the |
|
I think adding Is there a reason why |
|
I assume the I can figure out which yarn version corresponds to a version 6 |
|
You can bump the version of Line 54 in e03a75c
which should get picked up as the version of arrow-js-ffi/.github/workflows/test.yml Lines 19 to 20 in e03a75c
|
Per advice on PR, bumping the CI yarn version to match my local version, 4.9.4
Found the prettier command in the package.json scripts so I ran it :)
|
Would you be able to try uncommenting this test? arrow-js-ffi/tests/ffi.test.ts Lines 594 to 619 in e03a75c
It should be able to load this timestamp array defined in python: arrow-js-ffi/tests/pyarrow_generate_data.py Lines 108 to 117 in e03a75c
|
Per PR request, uncommented the "timestamp" test in ffi.test.ts and then recommented the console.log calls (as I assume they're unwanted with a succeeding test). Also "fixed" the test by replacing the word "test" with "it", which seemed more in-line with the rest of the file.
|
Awesome, thanks! |
Overview
Should resolve #129 by setting the
byteWidthcorrectly (according to the advice in the issue) to 8.Details
I realized that
getTimeByteWidthwas only being used by Timestamp and Duration, so I ended up just removing it and hardcoding thebyteWidth.I also wasn't able to successfully run the tests, but I did successfully run
yarn build(withArrayBufferLikewarnings). I can run the tests if necessary, but I may need a little guidance on therust-arrow-ffisetup.