GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls #37764

abandy · 2023-09-17T22:56:56Z

Swift Flight implementation behavior didn't match existing flight implementations. The updates in this PR have been tested against python/examples/flight/server.py except for getSchemaTest and doExchangeTest which are not implemented in server.py. I will follow up this PR with a cleanup refactoring PR.

This difference in behavior was found due to issue #37726.

Closes: [Swift][FlightSQL] Processing valid RecordBatch leads to invalid offset from body's data. #37726

github-actions · 2023-09-17T22:57:21Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2023-09-17T23:07:57Z

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

abandy · 2023-09-19T13:25:18Z

Hi @kou, I apologize for the large review. These changes are for a fix found in the attached issue. I plan to follow up this change with a PR that will fix formatting issues (planning to use swiftlint )

kou

How about using the following steps instead of "merging this PR without SwiftLint and applying SwiftLint with a separated PR"?

PR1: Add a lint CI job
- We can do it in https://github.com/apache/arrow/blob/main/ci/scripts/swift_test.sh
- We can apply SwiftLint in PR1 to fix CI failure
- We don't change any behavior in PR1 (Auto-generated changed by SwiftLint are only included in PR1)
PR2: Rebase this PR on main after PR1 is merged
- Formatted changes are easier to review
PR3: Add Swift to our integration test targets:
- Add tester_swift.py to https://github.com/apache/arrow/tree/main/dev/archery/archery/integration
- Enable the tester in https://github.com/apache/arrow/blob/main/dev/archery/archery/integration/runner.py#L534
- Setup Swift in https://github.com/apache/arrow/blob/main/ci/docker/conda-integration.dockerfile

BTW, could you add a list of changed behavior in the description of this PR? It seems that no response cases are changed.

kou · 2023-09-19T20:19:59Z

swift/Arrow/Sources/Arrow/ArrowReader.swift

@@ -200,4 +201,41 @@ public class ArrowReader {
            return .failure(.unknownError("Error loading file: \(error)"))
        }
    }
+
+    static public func MakeArrowReaderResult() -> ArrowReaderResult{


Suggested change

static public func MakeArrowReaderResult() -> ArrowReaderResult{

static public func MakeArrowReaderResult() -> ArrowReaderResult {

kou · 2023-09-19T20:21:50Z

swift/Arrow/Sources/Arrow/ArrowWriter.swift

-
+

Are this trailing spaces change and other similar changes intentional?

I wasn't too concerned with these style changes as I was going to follow up with a swiftlint PR. Since we are planning on running swiftlint on the code as a prior PR to this one, then these will be resolved (should be) when swiftlint is run against the files.

kou · 2023-09-19T20:24:58Z

swift/ArrowFlight/Sources/ArrowFlight/FlightClient.swift

+            for rc in recordBatchs {
+                switch writer.toMessage(rc) {


rc -> rb ("R"ecord "B"atch) or rc -> recordBatch may be better.

@abandy Could you check this?

Good catch, will update, thanks!

kou · 2023-09-19T20:27:38Z

swift/ArrowFlight/Sources/ArrowFlight/RecordBatchStreamReader.swift

+            descriptor = FlightDescriptor(flightData.flightDescriptor)
+            switch reader.fromMessage(dataHeader, dataBody: dataBody, result: result) {
+            case .success(()):
+                if(result.batches.count > 0 ) {


Suggested change

if(result.batches.count > 0 ) {

if result.batches.count > 0 {

kou · 2023-09-19T20:29:25Z

swift/ArrowFlight/Tests/ArrowFlightTests/FlightTest.swift

    }

    func getFlightInfo(_ request: ArrowFlight.FlightDescriptor) async throws -> ArrowFlight.FlightInfo {
-        return ArrowFlight.FlightInfo(Data())
+        let key = String(decoding: request.cmd, as: UTF8.self)
+        if(flights[key] != nil) {


Suggested change

if(flights[key] != nil) {

if flights[key] != nil {

kou · 2023-09-19T20:35:04Z

swift/ArrowFlight/Tests/ArrowFlightTests/FlightTest.swift

-        XCTAssertEqual(actionResults.count, 1)
-        XCTAssertEqual(String(bytes:actionResults[0].body, encoding: .utf8), "test_action result")
+        XCTAssertEqual(actionResults.count, 0)


It seems that this removes a test case for a DoAction that returns a result.
Is it intentional?

This was intentional but I will put test case back.

abandy · 2023-09-21T01:58:09Z

How about using the following steps instead of "merging this PR without SwiftLint and applying SwiftLint with a separated PR"?

PR1: Add a lint CI job

We can do it in https://github.com/apache/arrow/blob/main/ci/scripts/swift_test.sh

We can apply SwiftLint in PR1 to fix CI failure

We don't change any behavior in PR1 (Auto-generated changed by SwiftLint are only included in PR1)

PR2: Rebase this PR on main after PR1 is merged

Formatted changes are easier to review

PR3: Add Swift to our integration test targets:

Add tester_swift.py to https://github.com/apache/arrow/tree/main/dev/archery/archery/integration

Enable the tester in https://github.com/apache/arrow/blob/main/dev/archery/archery/integration/runner.py#L534

Setup Swift in https://github.com/apache/arrow/blob/main/ci/docker/conda-integration.dockerfile

BTW, could you add a list of changed behavior in the description of this PR? It seems that no response cases are changed.

Sounds good, I will give this a go.

abandy · 2023-10-25T15:13:53Z

@kou please review when you get a chance, thank you!

kou

+1

kou · 2023-10-25T23:26:18Z

Oh, it seems that we still want to keep working on this PR.

…ing impls

abandy · 2023-10-25T23:33:20Z

@kou I had found that I had missed the requested change to revert the doActionTest and updated accordingly. The PR is now ready. I apologize for any confusion.

kou · 2023-10-26T04:29:50Z

OK. I'll merge this.

abandy · 2023-10-26T10:42:29Z

thank you!

conbench-apache-arrow · 2023-10-27T12:06:52Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…xisting impls (apache#37764) Swift Flight implementation behavior didn't match existing flight implementations. The updates in this PR have been tested against python/examples/flight/server.py except for getSchemaTest and doExchangeTest which are not implemented in server.py. I will follow up this PR with a cleanup refactoring PR. This difference in behavior was found due to issue apache#37726. * Closes: apache#37726 Authored-by: Alva Bandy <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: Swift awaiting review Awaiting review labels Sep 17, 2023

abandy mentioned this pull request Sep 17, 2023

[Swift][FlightSQL] Processing valid RecordBatch leads to invalid offset from body's data. #37726

Closed

abandy changed the title ~~37726: [Swift] Update flight behavior to be similar to existing impls~~ GH-37726: [Swift] Update flight behavior to be similar to existing impls Sep 17, 2023

abandy force-pushed the 37726 branch 2 times, most recently from c988ebb to 24649c9 Compare September 18, 2023 22:15

abandy marked this pull request as ready for review September 19, 2023 13:23

abandy requested a review from kou as a code owner September 19, 2023 13:23

kou reviewed Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 19, 2023

abandy mentioned this pull request Oct 7, 2023

[Swift][CI] Add linting to CI job #38131

Closed

abandy force-pushed the 37726 branch from 24649c9 to eedd214 Compare October 25, 2023 13:14

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 25, 2023

abandy force-pushed the 37726 branch 5 times, most recently from 7ab44f0 to 94aa4b0 Compare October 25, 2023 15:01

kou changed the title ~~GH-37726: [Swift] Update flight behavior to be similar to existing impls~~ GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls Oct 25, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 25, 2023

abandy force-pushed the 37726 branch 2 times, most recently from 4736385 to 211fc12 Compare October 25, 2023 23:16

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 25, 2023

kou approved these changes Oct 25, 2023

View reviewed changes

abandy force-pushed the 37726 branch from 211fc12 to 4549951 Compare October 25, 2023 23:25

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 25, 2023

apacheGH-37726: [Swift] Update flight behavior to be similar to exist…

8b5d725

…ing impls

abandy force-pushed the 37726 branch from 4549951 to 8b5d725 Compare October 25, 2023 23:28

kou merged commit f2995b0 into apache:main Oct 26, 2023
6 checks passed

kou removed the awaiting merge Awaiting merge label Oct 26, 2023

abandy deleted the 37726 branch April 2, 2024 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls #37764

GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls #37764

abandy commented Sep 17, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Sep 17, 2023

github-actions bot commented Sep 17, 2023

abandy commented Sep 19, 2023 •

edited

Loading

kou left a comment

kou Sep 19, 2023

abandy Sep 21, 2023

kou Sep 19, 2023

abandy Sep 21, 2023

kou Sep 19, 2023

abandy Sep 21, 2023

kou Oct 25, 2023

abandy Oct 25, 2023

kou Sep 19, 2023

abandy Sep 21, 2023

kou Sep 19, 2023

abandy Sep 21, 2023

kou Sep 19, 2023

abandy Sep 21, 2023

abandy commented Sep 21, 2023

abandy commented Oct 25, 2023

kou left a comment

kou commented Oct 25, 2023

abandy commented Oct 25, 2023

kou commented Oct 26, 2023

abandy commented Oct 26, 2023

conbench-apache-arrow bot commented Oct 27, 2023

	static public func MakeArrowReaderResult() -> ArrowReaderResult{
	static public func MakeArrowReaderResult() -> ArrowReaderResult {

	if(result.batches.count > 0 ) {
	if result.batches.count > 0 {

GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls #37764

GH-37726: [Swift][FlightSQL] Update behavior to be similar to existing impls #37764

Conversation

abandy commented Sep 17, 2023 • edited by github-actions bot Loading

github-actions bot commented Sep 17, 2023

github-actions bot commented Sep 17, 2023

abandy commented Sep 19, 2023 • edited Loading

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandy commented Sep 21, 2023

abandy commented Oct 25, 2023

kou left a comment

Choose a reason for hiding this comment

kou commented Oct 25, 2023

abandy commented Oct 25, 2023

kou commented Oct 26, 2023

abandy commented Oct 26, 2023

conbench-apache-arrow bot commented Oct 27, 2023

abandy commented Sep 17, 2023 •

edited by github-actions bot

Loading

abandy commented Sep 19, 2023 •

edited

Loading