Skip to content

GH-39720: [Swift] Switch reader to use arrow field instead of proto for building arrays - #39721

Merged
kou merged 1 commit into
apache:mainfrom
abandy:GH-39720
Jan 27, 2024
Merged

GH-39720: [Swift] Switch reader to use arrow field instead of proto for building arrays#39721
kou merged 1 commit into
apache:mainfrom
abandy:GH-39720

Conversation

@abandy

@abandyabandy commented Jan 21, 2024

Copy link
Copy Markdown
Contributor

This PR updates the ArrowReaderHelper to use an ArrowField object for building an Array instead of a protobuf field obj. This removes leveraging protobuf from building out the Arrays and makes the code easier to reuse (like for the C Data Interface)

@abandy
abandy requested a review from kou as a code ownerJanuary 21, 2024 01:30
@abandyabandy changed the title MINOR:switch reader to use arrow field instead of proto for building arraysGH-39720:switch reader to use arrow field instead of proto for building arraysJan 21, 2024
@github-actionsgithub-actionsBot added the awaiting review Awaiting review label Jan 21, 2024
@abandyabandy changed the title GH-39720:switch reader to use arrow field instead of proto for building arraysGH-39720: [SWIFT] switch reader to use arrow field instead of proto for building arraysJan 21, 2024
@github-actions

Copy link
Copy Markdown

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

@abandyabandy changed the title GH-39720: [SWIFT] switch reader to use arrow field instead of proto for building arraysGH-39720: [Swift] switch reader to use arrow field instead of proto for building arraysJan 21, 2024
@abandy
abandyforce-pushed the GH-39720 branch 2 times, most recently from d95dbdf to 718e1c1CompareJanuary 21, 2024 01:47

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making this a normal func (instance method) not a static func?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about computing stride: in ArrowData.init() because ArrowData.init() can use the given ArrowType?

@github-actionsgithub-actionsBot added awaiting review Awaiting review awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 22, 2024
@github-actionsgithub-actionsBot added awaiting review Awaiting review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jan 23, 2024
@abandy

Copy link
Copy Markdown
ContributorAuthor

@kou I hope all is well. Please review again when you get a chance.

@koukou changed the title GH-39720: [Swift] switch reader to use arrow field instead of proto for building arraysGH-39720: [Swift] Switch reader to use arrow field instead of proto for building arraysJan 27, 2024
kou
kou approved these changes Jan 27, 2024

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou
kou merged commit 21ffd82 into apache:mainJan 27, 2024
@koukou removed the awaiting review Awaiting review label Jan 27, 2024
@github-actionsgithub-actionsBot added the awaiting merge Awaiting merge label Jan 27, 2024
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 21ffd82.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 35 possible false positives for unstable benchmarks that are known to sometimes produce them.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…roto for building arrays (apache#39721)
This PR updates the ArrowReaderHelper to use an ArrowField object for building an Array instead of a protobuf field obj. This removes leveraging protobuf from building out the Arrays and makes the code easier to reuse (like for the C Data Interface)
* Closes: apache#39720
Authored-by: Alva Bandy <abandy@live.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto 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.

[Swift] Change reader helper to use ArrowField instead of protobuf field obj for making and Array

2 participants

@abandy@kou