GH-40959: [JS] Store Timestamps in 64 bits #40960
Conversation
github-actions
bot
added
Component: JavaScript
awaiting committer review
… the only ones stored correctly
js/test/generate-test-data.ts
Outdated
| const mult = 86400 * multiple; | ||
| const data = new Int32Array(length * 2).fill(0); | ||
| const data = new BigInt64Array(length).fill(0n); | ||
| const data32 = createDate32(length, nullBitmap, values); |
There was a problem hiding this comment.
This is a bit weird, don't you want to create timestamp data that does not represent whole days?
There was a problem hiding this comment.
Good catch. I rewrote the timestamp generation logic to generate meaningful timestamps.
js/test/unit/builders/utils.ts
Outdated
| export const stringsNoNulls = (length = 20) => Array.from({ length }, (_) => randomString(1 + (Math.trunc(Math.random() * 19)))); | ||
| export const timestamp32sNoNulls = (length = 20, now = Math.trunc(Date.now() / 86400000)) => | ||
| export const timestampNoNulls = (length = 20, now = Math.trunc(Date.now() / 86400000)) => | ||
| Array.from({ length }, (_) => (Math.trunc(now + (rand() * 10000 * (rand() > 0.5 ? -1 : 1)))) * 86400000); |
There was a problem hiding this comment.
Given the scale of rand() * 10000 compared to now, this will never produce timestamps with a negative value, right?
There was a problem hiding this comment.
Good catch. I changed it to use 100000 which gives us negative numbers as well.
github-actions
bot
removed
the
awaiting committer review
github-actions
bot
added
awaiting change review
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 18876b2. There were 2 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
Merge after apache#40892. This pull request also changes Dates to return timestamps instead of Date instances (similar to Timestamps and for the same reason. * GitHub Issue: apache#40959
* adopt and document the new apache-arrow date encoding ref. apache/arrow#40960 * copy edit --------- Co-authored-by: Mike Bostock <[email protected]>
Merge after apache#40892. This pull request also changes Dates to return timestamps instead of Date instances (similar to Timestamps and for the same reason. * GitHub Issue: apache#40959
4 participants
Merge after #40892.
This pull request also changes Dates to return timestamps instead of Date instances (similar to Timestamps and for the same reason.