GH-40959: [JS] Store Timestamps in 64 bits #40960
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Component: JavaScript
awaiting committer review
… the only ones stored correctly
pitrou
reviewed
Apr 4, 2024
pitrou
reviewed
Apr 4, 2024
pitrou
reviewed
Apr 4, 2024
pitrou
reviewed
Apr 4, 2024
js/test/generate-test-data.ts
Outdated
| } | ||
| }); | ||
| return data; | ||
| } | ||
|
|
||
| function createTimestamp(length: number, nullBitmap: Uint8Array, multiple: number, values: (number | null)[] = []) { | ||
| 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.
pitrou
reviewed
Apr 4, 2024
js/test/unit/builders/utils.ts
Outdated
| @@ -32,22 +32,14 @@ const randnulls = <T, TNull = null>(values: T[], n: TNull = <any>null) => values | |||
| export const randomBytes = (length: number) => fillRandom(Uint8Array, length); | |||
|
|
|||
| 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.
pitrou
reviewed
Apr 4, 2024
github-actions
bot
removed
the
awaiting committer review
github-actions
bot
added
awaiting change review
trxcllnt
approved these changes
Apr 16, 2024
|
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. |
domoritz
added a commit
to domoritz/arrow
that referenced
this pull request
Apr 17, 2024
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
raulcd
pushed a commit
that referenced
this pull request
Apr 29, 2024
tolleybot
pushed a commit
to tmct/arrow
that referenced
this pull request
May 2, 2024
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
tolleybot
pushed a commit
to tmct/arrow
that referenced
this pull request
May 4, 2024
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
rok
pushed a commit
to tmct/arrow
that referenced
this pull request
May 8, 2024
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
rok
pushed a commit
to tmct/arrow
that referenced
this pull request
May 8, 2024
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
Fil
added a commit
to observablehq/framework
that referenced
this pull request
May 21, 2024
Fil
added a commit
to observablehq/framework
that referenced
this pull request
May 21, 2024
* adopt and document the new apache-arrow date encoding ref. apache/arrow#40960 * copy edit --------- Co-authored-by: Mike Bostock <mbostock@gmail.com>
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
May 25, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge after #40892.
This pull request also changes Dates to return timestamps instead of Date instances (similar to Timestamps and for the same reason.