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

GH-35788: [Swift] bug fixes and change reader/writer to user Result type #35774

Merged
merged 1 commit into from
May 28, 2023
Merged

GH-35788: [Swift] bug fixes and change reader/writer to user Result type #35774

merged 1 commit into from
May 28, 2023

Conversation

abandy
Copy link
Contributor

@abandy abandy commented May 26, 2023 •
edited by github-actions bot

Changes:

  • Changed Reader and Writer to use Result<T, Error> instead of throwing. Since the Error is an enum the handling of the error is enforced.
  • Update RecordBatch to accept only a single array per column instead of chunked arrays (which could cause problems when writing out)
  • Added ability for Table to be created from RecordBatches
  • Fixed a bug with indexing in Chunked array with many arrays
  • Added tests for the above changes.
  • Switched ValidationErrors to ArrowError and added additional ArrowError values.

@github-actions github-actions bot added the awaiting review Awaiting review label May 26, 2023
@kou
Copy link
Member

kou commented May 26, 2023

Could you open a new issue for this because this is not a MINOR change?

See also: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes

@abandy
Copy link
Contributor Author

abandy commented May 26, 2023

Could you open a new issue for this because this is not a MINOR change?

See also: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes

I see :). I will open an issue, thank you!

@abandy abandy changed the title MINOR: [Swift] bug fixes and change reader/writer to user Result type GH-35788: [Swift] bug fixes and change reader/writer to user Result type May 26, 2023
@github-actions
Copy link

@github-actions
Copy link

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

kou
kou reviewed May 26, 2023
View reviewed changes
swift/Arrow/Sources/Arrow/ArrowArray.swift Outdated
public let type: ArrowType.Info
public let length: UInt
public let nullCount: UInt
public let holder: Any
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming this to array?

Suggested change
public let holder: Any
public let array: Any

I feel that ArrowArrayHolder.holder is strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, yeah, good catch, I will update.

swift/Arrow/Sources/Arrow/ArrowArray.swift Outdated
public static func makeArrowColumn(_ field: ArrowField, holders: [ArrowArrayHolder]) -> Result<ArrowColumn, ArrowError> {
do {
return .success(try holders[0].getArrowColumn(field, holders))
}catch {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}catch {
} catch {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update.

swift/Arrow/Sources/Arrow/ArrowReader.swift Outdated
recordBatchs.append(recordBatch)
do {
let recordBatch = try loadRecordBatch(message, schema: footer.schema!,
data: fileData, messageEndOffset: messageEndOffset).get()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data: fileData, messageEndOffset: messageEndOffset).get()
data: fileData, messageEndOffset: messageEndOffset).get()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 26, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 26, 2023
kou
kou approved these changes May 28, 2023
View reviewed changes
Copy link
Member

@kou kou left a comment

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 d14b42a into apache:main May 28, 2023
6 of 7 checks passed
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 28, 2023
@ursabot
Copy link

ursabot commented May 31, 2023

Benchmark runs are scheduled for baseline = 130f9e9 and contender = d14b42a. d14b42a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d14b42ac ec2-t3-xlarge-us-east-2
[Failed] d14b42ac test-mac-arm
[Finished] d14b42ac ursa-i9-9960x
[Failed] d14b42ac ursa-thinkcentre-m75q
[Finished] 130f9e98 ec2-t3-xlarge-us-east-2
[Failed] 130f9e98 test-mac-arm
[Finished] 130f9e98 ursa-i9-9960x
[Failed] 130f9e98 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented May 31, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kou kou kou approved these changes

Assignees
No one assigned
Labels
awaiting merge Awaiting merge
Projects
None yet
Milestone
No milestone
3 participants
@abandy @kou @ursabot