-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-40959: [JS] Store Timestamps in 64 bits #40960
Conversation
… the only ones stored correctly
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I rewrote the timestamp generation logic to generate meaningful timestamps.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I changed it to use 100000
which gives us negative numbers as well.
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
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
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
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
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 <mbostock@gmail.com>
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
Merge after #40892.
This pull request also changes Dates to return timestamps instead of Date instances (similar to Timestamps and for the same reason.