GH-37938: [Swift] Add initial C data interface implementation #41342

abandy · 2024-04-23T00:01:46Z

Continuation for PR: #39091

This add an initial implementation of the C Data interface for swift. During development it was found that null count was not being properly maintained on the arrow buffers and this change is included as well. Also some minor refactoring was done to existing sources to enable this feature.

This has been tested from Swift calling into C to import data but not from Swift to C exporting data. Test is currently ongoing.

GitHub Issue: [Swift] Implement FFI support #37938

github-actions · 2024-04-23T00:02:16Z

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

abandy · 2024-04-23T20:55:03Z

The go test for the C data interface are currently not included in CI testing. I plan to update this in a future PR.

dbtsai · 2024-04-23T23:39:48Z

It’s great to add those interfaces to add Swift support.

Cc @andygrove @viirya

Thanks!

dbtsai · 2024-04-23T23:40:20Z

ci/scripts/swift_test.sh

@@ -40,4 +40,4 @@ popd
 source_dir=${1}/swift/ArrowFlight
 pushd ${source_dir}
 swift test
-popd
+popd


Unnecessary change

abandy · 2024-04-24T00:13:00Z

It’s great to add those interfaces to add Swift support.

Cc @andygrove @viirya

Thanks!

Np, thank you!

kou · 2024-04-24T00:17:01Z

@dbtsai Are you familiar with Swift? If so, could you also review this carefully?

viirya · 2024-04-24T01:09:05Z

swift/Arrow/Package.swift

+            name: "ArrowC", // your C/C++ library's name
+            path: "Sources/ArrowC" // your path to the C/C++ library


These comments look like from some examples and maybe unrelated here?

Good catch! I will remove.

viirya · 2024-04-24T02:27:24Z

swift/Arrow/Sources/Arrow/ArrowCExporter.swift

+        cArray.n_children = 0
+        cArray.children = nil
+        cArray.dictionary = nil


Hmm, is this for TODO later? Otherwise I think n_children should not be zero always? And also for children and dictionary.

The Swift version of Arrow currently doesn't support having children (Struct type) or dictionaries. I will add a comment mentioning this so it can be updated when those constructs are added.

viirya · 2024-04-24T02:29:17Z

swift/Arrow/Sources/Arrow/ArrowCExporter.swift

+                let arraySchema = data!.pointee
+                let exportId = Int(bitPattern: arraySchema.private_data)
+                guard ArrowCExporter.exportedData[exportId] != nil else {
+                    fatalError("Export schema not found with id \(exportId)")


Does fatalError actually do something like C exit?

I think the behavior is the same. Description from Swift docs for fatalError: Unconditionally prints a given message and stops execution.

abandy · 2024-04-28T23:11:28Z

@kou and @pitrou Please review when you get a chance. This is very similar to the previous PR(#39091) for this enhancement that was closed but with the updates from @pitrou previous comments. Thank you!

kou · 2024-05-01T09:20:03Z

swift/Arrow/Package.swift

Could you add this file to swiftlint targets?

I also added ArrowFlight and CDataWGo Package files as well, thanks!

abandy · 2024-05-15T13:30:08Z

@kou I have made the requested changes. Please review again.

pitrou · 2024-05-16T14:55:29Z

@abandy Could you please rebase from git main, if not recently done, so that we get cleaner CI results?

pitrou · 2024-05-16T15:40:36Z

swift/Arrow/Sources/Arrow/ArrowCExporter.swift

+        }
+    }
+
+    private static var exportedData = [Int: ExportData]()


Is this container cleaned up at shutdown? I'm wondering what happens if a release callback is called after this container is cleaned up. Presumably the release callback would crash because of the fatalError?

This is not necessarily a problem, but I'm curious :-)

Yes, the memory should be released during shutdown. The container should be valid in the process that loaded the library until the process has been shutdown.

Yes, my question was more along the lines of: what happens if the release callback is called after finalization of Swift internals has started? Granted, we might not be able to do much about it, but it's probably good to keep this in mind (and/or add a comment about it).

pitrou · 2024-05-16T15:46:38Z

swift/Arrow/Sources/Arrow/ArrowCExporter.swift

+            self.arrowType = arrowType
+            // keeping the name str to ensure the cstring buffer remains valid
+            self.name = name
+            self.arrowTypeName = (try arrowType.cDataFormatId as NSString).utf8String!


This is only ok because arrowType.cDataFormatId always returns a static string, right?
Otherwise you would also have to store arrowType.cDataFormatId in ExportSchema so that its buffer remains valid.

Yes, that is correct.

Ok, I suppose that's not very future-proof, then? Some types such as timestamp, union... have dynamic format strings which embody the type's parameters. You may want to add a comment to remind readers about this?

(or perhaps simply arrange to keep the arrowType.cDataFormatId result alive in the ExportSchema class)

I have a PR ready for Struct types and plan to follow up with Unions afterward. I was going to address this issue when I added support for these types to the C data interface in a future PR but I will make this change now so it is one less thing that will be an issue at that time. Thank you!

pitrou · 2024-05-16T15:48:41Z

swift/Arrow/Sources/Arrow/ArrowCImporter.swift

+            return .failure(.invalid("Children currently not supported"))
+        } else if cSchema.dictionary != nil {
+            return .failure(.invalid("Dictinoary types currently not supported"))


Should also call ArrowCImporter.release(cSchema) here?

I see, agreed, I will update.

pitrou · 2024-05-16T15:50:13Z

swift/Arrow/Sources/Arrow/ArrowCImporter.swift

+        case .success(let holder):
+            return .success(ImportArrayHolder(holder, cArrayPtr: cArrayPtr))
+        case .failure(let err):
+            return .failure(err)


Should also call ArrowCImporter.release here?

abandy · 2024-05-23T19:48:17Z

@pitrou and @kou I have made the requested changes. Please review again. I have 2 PRs waiting for some of theses changes (one is for adding struct type) and it would be great if we could get this merged so I can submit them. Thank you!

kou

In general, smaller PRs are easy to review/merge than one large PR.
If you want to proceed quickly as much as possible, could you use the smaller PRs approach next time like you will do for struct type?
For example, we might be able to choose the following approach for this case:

Implement ArrowSchema export for 1 simple type such as bool.
Implement ArrowSchema import for 1.
Implement ArrowSchema export/import for one more simple type such as int8.
...

FYI: MATLAB uses the small PRs approach: https://github.com/apache/arrow/issues?q=is%3Aissue+matlab+%22C+data+interface%22

kou · 2024-05-24T00:13:56Z

swift/.swiftlint.yml

@@ -16,10 +16,15 @@
 # under the License.

 included:
+


Suggested change

abandy · 2024-05-24T00:56:21Z

In general, smaller PRs are easy to review/merge than one large PR. If you want to proceed quickly as much as possible, could you use the smaller PRs approach next time like you will do for struct type? For example, we might be able to choose the following approach for this case:

Implement ArrowSchema export for 1 simple type such as bool.

Implement ArrowSchema import for 1.

Implement ArrowSchema export/import for one more simple type such as int8.

...

FYI: MATLAB uses the small PRs approach: https://github.com/apache/arrow/issues?q=is%3Aissue+matlab+%22C+data+interface%22

Hi @kou, thank you example on the smaller project. I will definitely follow that format for the struct type change! Thank you very much!

pitrou

@abandy This LGTM in general, I've just posted two comments about potential corner cases. I'll let @kou give the final go.

kou

+1

I added some minor comments but I'll merge this. If we need to work on these comments, let's work on them as follow-up tasks.

kou · 2024-05-29T02:47:46Z

swift/Arrow/Sources/ArrowC/include/ArrowCData.h

+#define ARROW_FLAG_NULLABLE 2
+#define ARROW_FLAG_MAP_KEYS_SORTED 4
+
+#include <stdio.h>


Can we remove this?

kou · 2024-05-29T02:47:57Z

swift/Arrow/Sources/ArrowC/include/ArrowCData.h

+#define ARROW_FLAG_MAP_KEYS_SORTED 4
+
+#include <stdio.h>
+#include <stdlib.h>


Can we move this to ArrowCData.c?

kou · 2024-05-29T02:49:16Z

swift/Arrow/Sources/ArrowC/include/ArrowCData.h

+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>  // Must have this!


Suggested change

#include <stdint.h> // Must have this!

#include <stdint.h> // For int64_t

kou · 2024-05-29T02:55:43Z

swift/CDataWGo/go.mod

+require (
+	github.com/andybalholm/brotli v1.0.5 // indirect
+	github.com/apache/arrow/go/v12 v12.0.1 // indirect
+	github.com/apache/arrow/go/v16 v16.0.0-20240203105949-22f2cfd1e1eb // indirect


Is this really indirect dependency?

abandy · 2024-05-29T16:22:07Z

+1

I added some minor comments but I'll merge this. If we need to work on these comments, let's work on them as follow-up tasks.

Thank you @kou, @pitrou, @dbtsai, @viirya for the reviews!!

I will create a MINOR pr for the comments added by @kou.

### Rationale for this change Follow up changes requested in the #41342 ### What changes are included in this PR? Update includes header file and go dependencies changes related to the C Data interface changes. Authored-by: Alva Bandy <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

conbench-apache-arrow · 2024-06-06T04:14:05Z

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

There were no benchmark performance regressions. 🎉

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

abandy requested review from assignUser, kou and raulcd as code owners April 23, 2024 00:01

github-actions bot added Component: Swift awaiting review Awaiting review labels Apr 23, 2024

abandy force-pushed the GH-37938 branch 4 times, most recently from 461cf90 to 453cf4f Compare April 23, 2024 16:22

dbtsai reviewed Apr 23, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 23, 2024

abandy force-pushed the GH-37938 branch 2 times, most recently from 739c452 to c269261 Compare April 24, 2024 00:11

viirya reviewed Apr 24, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review and removed awaiting committer review Awaiting committer review labels Apr 24, 2024

abandy force-pushed the GH-37938 branch from c269261 to 7d2591e Compare April 24, 2024 10:49

github-actions bot added awaiting review Awaiting review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Apr 24, 2024

abandy force-pushed the GH-37938 branch from 7d2591e to dc1534d Compare April 28, 2024 23:07

kou reviewed May 1, 2024

View reviewed changes

pitrou reviewed May 16, 2024

View reviewed changes

abandy force-pushed the GH-37938 branch from 2d99636 to afc1c13 Compare May 18, 2024 17:24

kou reviewed May 24, 2024

View reviewed changes

swift/.swiftlint.yml Outdated

@@ -16,10 +16,15 @@

# under the License.

included:

Copy link

Member

kou May 24, 2024

Choose a reason for hiding this comment

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

Suggested change

abandy reacted with thumbs up emoji

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 24, 2024

abandy force-pushed the GH-37938 branch from afc1c13 to 869898c Compare May 24, 2024 00:59

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 24, 2024

pitrou approved these changes May 28, 2024

View reviewed changes

apacheGH-37938: [Swift] initial impl of C Data interface

bf12256

abandy force-pushed the GH-37938 branch from 869898c to bf12256 Compare May 28, 2024 19:48

kou approved these changes May 29, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 29, 2024

kou changed the title ~~GH-37938: [Swift] initial impl of C Data interface~~ GH-37938: [Swift] Add initial C data interface implementation May 29, 2024

kou merged commit 4d524eb into apache:main May 29, 2024
53 of 55 checks passed

kou removed the awaiting merge Awaiting merge label May 29, 2024

kou mentioned this pull request May 29, 2024

[Swift] Implement FFI support #37938

Closed

abandy mentioned this pull request May 29, 2024

MINOR: [Swift] cleanup some go and C++ artifacts #41878

Merged

abandy deleted the GH-37938 branch June 10, 2024 14:01

		name: "ArrowC", // your C/C++ library's name
		path: "Sources/ArrowC" // your path to the C/C++ library

	#include <stdint.h> // Must have this!
	#include <stdint.h> // For int64_t

GH-37938: [Swift] Add initial C data interface implementation #41342

GH-37938: [Swift] Add initial C data interface implementation #41342

Conversation

abandy commented Apr 23, 2024 • edited by kou Loading

github-actions bot commented Apr 23, 2024

abandy commented Apr 23, 2024 • edited Loading

dbtsai commented Apr 23, 2024

Choose a reason for hiding this comment

abandy commented Apr 24, 2024

kou commented Apr 24, 2024

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 Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandy commented May 15, 2024

pitrou commented May 16, 2024

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 May 23, 2024

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandy commented May 24, 2024

pitrou left a comment

Choose a reason for hiding this comment

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

abandy commented May 29, 2024

conbench-apache-arrow bot commented Jun 6, 2024

abandy commented Apr 23, 2024 •

edited by kou

Loading

abandy commented Apr 23, 2024 •

edited

Loading