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.

Sign up for GitHub

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

Jump to bottom

[Go] Code should avoid calling schema.Fields where possible #38918

Closed
asubiotto opened this issue Nov 28, 2023 · 0 comments · Fixed by #38919
Closed

[Go] Code should avoid calling schema.Fields where possible #38918

asubiotto opened this issue Nov 28, 2023 · 0 comments · Fixed by #38919
Assignees
@asubiotto
Labels
Component: Go Type: enhancement
Milestone
15.0.0

Comments

@asubiotto
Copy link
Contributor

Describe the enhancement requested

#35307 changed schema.Fields to return a copy of the fields for safety:

arrow/go/arrow/schema.go

Line 198 in c614014

fields := make([]Field, len(sc.fields))

However, a big part of the codebase continues to use schema.Fields assuming it is zero-allocation. We should review all callsites and change to using NumFields in cases where we only need the number of fields and looping over the field indices rather than the result of schema.Fields.

Component(s)

Go
@github-actions github-actions bot mentioned this issue Nov 28, 2023
Merged
zeroshade pushed a commit that referenced this issue Nov 28, 2023
@asubiotto
GH-38918: [Go] Avoid schema.Fields allocations in some places (#38919)
82be255 
### Rationale for this change

Unnecessary allocations.

### What changes are included in this PR?

This PR is split into several commits. The first addresses allocations in the `dictutils` package, the second adds `NumFields` to `NestedType` so that the third commit, which is a purely mechanical change from `len(type.Fields())` to `type.NumFields` to avoid allocations in these specific cases can pass tests with no further changes.

The last commit removes some Fields allocations that specifically hurt our project. Note that this is not an all-encompassing change (therefore this PR should probably not close the linked issue).

### Are these changes tested?

These changes are implicitly tested by the existing test-suite. No functionality has been changed and they should be invisible to the user.

### Are there any user-facing changes?

No.

* Addresses: #38918
* Closes: #38918

Authored-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 15.0.0 milestone Nov 28, 2023
@kou kou changed the title [Go]: Code should avoid calling schema.Fields where possible [Go] Code should avoid calling schema.Fields where possible Nov 29, 2023
asubiotto added a commit to asubiotto/arrow that referenced this issue Nov 29, 2023
@asubiotto
apacheGH-38918: [Go] Avoid schema.Fields allocations in some places (a…
e617a5b 
…pache#38919)

### Rationale for this change

Unnecessary allocations.

### What changes are included in this PR?

This PR is split into several commits. The first addresses allocations in the `dictutils` package, the second adds `NumFields` to `NestedType` so that the third commit, which is a purely mechanical change from `len(type.Fields())` to `type.NumFields` to avoid allocations in these specific cases can pass tests with no further changes.

The last commit removes some Fields allocations that specifically hurt our project. Note that this is not an all-encompassing change (therefore this PR should probably not close the linked issue).

### Are these changes tested?

These changes are implicitly tested by the existing test-suite. No functionality has been changed and they should be invisible to the user.

### Are there any user-facing changes?

No.

* Addresses: apache#38918
* Closes: apache#38918

Authored-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
asubiotto added a commit to asubiotto/arrow that referenced this issue Nov 29, 2023
@asubiotto
apacheGH-38918: [Go] Avoid schema.Fields allocations in some places (a…
b321865 
…pache#38919)

### Rationale for this change

Unnecessary allocations.

### What changes are included in this PR?

This PR is split into several commits. The first addresses allocations in the `dictutils` package, the second adds `NumFields` to `NestedType` so that the third commit, which is a purely mechanical change from `len(type.Fields())` to `type.NumFields` to avoid allocations in these specific cases can pass tests with no further changes.

The last commit removes some Fields allocations that specifically hurt our project. Note that this is not an all-encompassing change (therefore this PR should probably not close the linked issue).

### Are these changes tested?

These changes are implicitly tested by the existing test-suite. No functionality has been changed and they should be invisible to the user.

### Are there any user-facing changes?

No.

* Addresses: apache#38918
* Closes: apache#38918

Authored-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@asubiotto @dgreiss
apacheGH-38918: [Go] Avoid schema.Fields allocations in some places (a…
6beb9df 
…pache#38919)

### Rationale for this change

Unnecessary allocations.

### What changes are included in this PR?

This PR is split into several commits. The first addresses allocations in the `dictutils` package, the second adds `NumFields` to `NestedType` so that the third commit, which is a purely mechanical change from `len(type.Fields())` to `type.NumFields` to avoid allocations in these specific cases can pass tests with no further changes.

The last commit removes some Fields allocations that specifically hurt our project. Note that this is not an all-encompassing change (therefore this PR should probably not close the linked issue).

### Are these changes tested?

These changes are implicitly tested by the existing test-suite. No functionality has been changed and they should be invisible to the user.

### Are there any user-facing changes?

No.

* Addresses: apache#38918
* Closes: apache#38918

Authored-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@asubiotto asubiotto

Labels
Component: Go Type: enhancement
Projects
None yet
Milestone
15.0.0
Development

Successfully merging a pull request may close this issue.

GH-38918: [Go] Avoid schema.Fields allocations in some places asubiotto/arrow
2 participants
@zeroshade @asubiotto