Skip to content
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

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

swallez
Copy link
Member

@swallez swallez commented Oct 2, 2024

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.

Copy link

github-actions bot commented Oct 2, 2024

⚠️ GitHub issue #30863 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 2, 2024
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Oct 2, 2024
@domoritz domoritz merged commit b754d5a into apache:main Oct 3, 2024
11 checks passed
@domoritz domoritz modified the milestone: 18.0.0 Oct 3, 2024
Copy link

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.

@swallez swallez deleted the singleton-structrow-proxy branch October 3, 2024 09:32
@raulcd
Copy link
Member

raulcd commented Oct 3, 2024

@github-actions crossbow submit verify-rc-source-integration-linux-ubuntu-22.04-amd64

Copy link

github-actions bot commented Oct 3, 2024

Revision: 71590cd

Submitted crossbow builds: ursacomputing/crossbow @ actions-4738f42be4

Task Status
verify-rc-source-integration-linux-ubuntu-22.04-amd64 GitHub Actions

@raulcd
Copy link
Member

raulcd commented Oct 3, 2024

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)

@domoritz
Copy link
Member

domoritz commented Oct 3, 2024

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?

@raulcd
Copy link
Member

raulcd commented Oct 3, 2024

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.
This job has started failing in the last nightlies. I do not think this PR is the culprit but the integration jobs fail on Ubuntu and in general always around JS (either producing or consuming)

@domoritz
Copy link
Member

domoritz commented Oct 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[JS] Use a flywheel for struct row
4 participants