-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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-39257: [JS] LargeBinary #39258
GH-39257: [JS] LargeBinary #39258
Conversation
protected _flushPending(pending: Map<number, Uint8Array | undefined>, pendingLength: number) { | ||
const offsets = this._offsets; | ||
const data = this._values.reserve(pendingLength).buffer; | ||
let offset = 0; | ||
for (const [index, value] of pending) { | ||
if (value === undefined) { | ||
offsets.set(index, BigInt(0)); | ||
} else { | ||
const length = value.length; | ||
data.set(value, offset); | ||
offsets.set(index, BigInt(length)); | ||
offset += length; | ||
} | ||
} | ||
} | ||
} |
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.
Do you want to activate the placeholder in builder/largeutf8.ts in this PR?
Seems like you could probably do the same for getByteLength, too.
(LargeUtf8Builder.prototype as any)._flushPending = (LargeBinaryBuilder.prototype as any)._flushPending;
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.
Yes, just updated the pull request.
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.
Looks good to me, fwiw, except for the duplicated code between the utf8 and binary builders. The new unification stuff is nice.
Oh good catch. I forgot to remove that duplicate code. |
CC @kylebarron |
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.
lgtm!
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 4ec6544. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Merge after apache#39249 * Closes: apache#39257
Merge after apache#39249 * Closes: apache#39257
Merge after #39249