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-36384: [Go] Schema: NumFields #36365

Merged
merged 2 commits into from
Jul 2, 2023
Merged

GH-36384: [Go] Schema: NumFields #36365

merged 2 commits into from
Jul 2, 2023

Conversation

thorfour
Copy link
Contributor

@thorfour thorfour commented Jun 28, 2023

Rationale for this change

Previously if one wanted to iterate over the fields in the schema you would call the Fields() function and just iterate over the slice. However, due to this commit there is now an allocation and copy that happens when that's called. So to iterate over the fields without allocations one now must use the Field(i int) method; however that means a user must already know exactly how many fields are in the schema which isn't possible today.

This adds a simple NumFields() int method that returns the number of fields in a schema to allow a user to iterate over all the fields without having to copy them.

What changes are included in this PR?

Expose the number of fields in a schema for iteration over fields.

Single function added NumFields() int to schema

Are these changes tested?

N/A

Are there any user-facing changes?

Yes this is a new API

Expose the number of fields in a schema for iteration over fields.
@kou
Copy link
Member

kou commented Jun 28, 2023

Could you open a new issue for non-MINOR change instead of using MINOR: title?
See also our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

@thorfour
Copy link
Contributor Author

Could you open a new issue for non-MINOR change instead of using MINOR: title? See also our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

Ah my bad, missed that. Will open

@thorfour thorfour changed the title MINOR: [Go] Schema: NumFields [Go] Schema: NumFields Jun 29, 2023
@thorfour
Copy link
Contributor Author

This addresses #36384

@kou kou changed the title [Go] Schema: NumFields GH-36384: [Go] Schema: NumFields Jun 29, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jun 29, 2023
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 2, 2023
@zeroshade zeroshade merged commit 575b095 into apache:main Jul 2, 2023
25 checks passed
@thorfour thorfour deleted the num-fields branch July 3, 2023 13:45
@conbench-apache-arrow
Copy link

Conbench analyzed the 6 benchmark runs on commit 575b0957.

There were 8 benchmark results indicating a performance regression:

The full Conbench report has more details.

westonpace pushed a commit to westonpace/arrow that referenced this pull request Jul 7, 2023
### Rationale for this change

Previously if one wanted to iterate over the fields in the schema you would call the `Fields()` function and just iterate over the slice. However, due to [this commit](rtpsw@802674b) there is now an allocation and copy that happens when that's called. So to iterate over the fields without allocations one now must use the `Field(i int)` method; however that means a user must already know exactly how many fields are in the schema which isn't possible today.

This adds a simple `NumFields() int` method that returns the number of fields in a schema to allow a user to iterate over all the fields without having to copy them. 

### What changes are included in this PR?

Expose the number of fields in a schema for iteration over fields.

Single function added `NumFields() int` to schema

### Are these changes tested?

N/A

### Are there any user-facing changes?

Yes this is a new API

* Closes: apache#36384

Authored-by: thorfour <me@thor-hansen.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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Go] Expose number of fields in schema
3 participants