-
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-30863: [JS] Use a singleton StructRow proxy handler #44289
Conversation
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b754d5a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 37 possible false positives for unstable benchmarks that are known to sometimes produce them. |
@github-actions crossbow submit verify-rc-source-integration-linux-ubuntu-22.04-amd64 |
Revision: 71590cd Submitted crossbow builds: ursacomputing/crossbow @ actions-4738f42be4
|
I am not sure if: Is related but the failures seem to be around JS integration and this is the only commit that has been introduced lately (the commit doesn't seem like has anything to do with the failures to be honest but worth pointing out) |
All the ci checks passed in this pull request so shouldn't this indicate that this change is not the culprit and since it's the last change from js that the issue is somewhere else? |
Arrow's CI is quite extended and, for example, integration tests are not run on normal CI but as nightly jobs so sometimes things are merged and they break on the nightlies. It's not ideal but a compromise as resources for GH are limited and we struggled in the past with extreme queues. |
I see. Thanks for the additional context. I don't have cycles to debug the ci right now and we didn't make any changes to js recently besides this change. |
Rationale for this change
Fixes #30863 by using a singleton proxy handler in
StructRow
's constructor. Since the handler is stateless, there is no need to create a new instance for each row.What changes are included in this PR?
Refactoring
StructRow
's constructor to extract the proxy handler.Are these changes tested?
No additional tests since this is an internal refactoring, but
yarn test
runs successfully.Are there any user-facing changes?
No.