GH-34858: [Swift] Initial reader impl #34842

abandy · 2023-04-02T03:54:30Z

Initial check in for the swift arrow reader impl
bug fixes found during reader testing
class/method access modifier changes (mostly from internal to public)

Closes: [Swift] Add reader support #34858

github-actions · 2023-04-02T03:54:51Z

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-04-03T12:55:05Z

Closes: [Swift] Add reader support #34858

github-actions · 2023-04-03T12:55:09Z

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

kou · 2023-04-03T19:58:21Z

swift/Arrow/Sources/Arrow/File_generated.swift

Should we add these generated files to this repository?
Can we generate them in build time instead of adding them?

I guess we could generate them during build/test time.

I misread the intent of this change. I believe these generated files need to be included in the repo. If someone wants to use this lib in there code then they would reference this package in their package.swift file and would need these generated files to be part of the code base.

OK. Then could you add a script to generate these files so that we can regenerate them when we update FlatBuffers compiler?
And could you also add the Apache 2.0 license header to these generated files in the script?

kou · 2023-04-03T20:11:40Z

swift/Arrow/Tests/ArrowTests/testdata.arrow

How did you create these test data?
If you created them by using other Apache Arrow implementation such as C++ implementation, how about adding a program that generates them and generating them in test time instead of adding these test data?

I created them using python but can switch to C++ and build executable.

Added the test data to the test as base64 encoded strings. We can remove this data and generate it once the FileWriter has been implemented?

Added the test data to the test as base64 encoded strings.

It'll work for now but it will not work (too large data) when we have more supported patterns.
How about generate_test_data.py or something and generating test data in test time?

We can remove this data and generate it once the FileWriter has been implemented?

Yes.

Do we need to install python into the docker image to build the test data or will it run before the image is built and be pulled into the image

Could you use the former approach?
If we use the former approach, we don't need PyArrow available environment on host.

Gotcha, will do.

kou · 2023-04-03T20:14:25Z

swift/Arrow/Sources/Arrow/ChunkedArray.swift

+        var localIndex = index
+        var arrayIndex = 0;
+        var len: UInt = arrays[arrayIndex].length
+        while(localIndex > len) {


Is a space missing here?

Suggested change

while(localIndex > len) {

while (localIndex > len) {

Good catch! I will remove the parenthesis.

kou · 2023-04-03T20:25:26Z

swift/Arrow/Sources/Arrow/ArrowReader.swift

+let FILEMARKER = "ARROW1"
+let CONTINUATIONMARKER = -1
+
+public class ArrowReader {


We have "streaming format" https://arrow.apache.org/docs/format/Columnar.html#ipc-streaming-format and "file format" https://arrow.apache.org/docs/format/Columnar.html#ipc-file-format .
It seems that this is a reader for "file format".
We may want to add "file" to this class name such as ArrowFileReader.

This class contains a method for fromFile and fromStream. AFAIK the file format wraps the stream format with some extra data, so the fromFile method basically just unwraps the Stream formatted message and sends that to the fromStream method. Does that sound correct?

the file format wraps the stream format with some extra data

Correct.

But the file format can random read:

https://arrow.apache.org/docs/format/Columnar.html#ipc-file-format

This enables random access to any record batch in the file.

The streaming format needs to read from start to end. Because delta dictionary replacement is supported in the streaming format.

See also: https://arrow.apache.org/docs/format/Columnar.html#ipc-file-format

In the file format, there is no requirement that dictionary keys should be defined in a DictionaryBatch before they are used in a RecordBatch, as long as the keys are defined somewhere in the file. Further more, it is invalid to have more than one non-delta dictionary batch per dictionary ID (i.e. dictionary replacement is not supported). Delta dictionaries are applied in the order they appear in the file footer.

This class contains a method for fromFile and fromStream.

I see. I misread the implementation. Sorry. I think that it's OK with the current implementation as the first step. We can improve it in follow-up pull requests.

abandy · 2023-04-18T13:18:51Z

@kou I think I made all the requested updates. Please review again and approve if all looks good.

kou · 2023-04-18T20:53:02Z

swift/Arrow/swift-datagen

How did you generate this binary?

Generated with golang

I see.
Could you add source code instead of built binary so that anyone can update the program?

I actually didn't keep the code :). This is only temporary until we get the writer working, so maybe this is ok until then?

Hmm.
Does it mean that your next pull request implements double/bool writer? And the following pull requests implement reader and writer at once instead of implementing reader with test data then implementing writer and remove the test data like this, right?
If so, we can embed the two test data into our test in base64 as you did because they are temporary, we don't update them and we don't add other test data.

If we still keep implementing reader with test data then implementing writer and remove the test data style, I think that we should add source code of test data generator program so that anyone can join the Swift implementation development.
See also: The Apache Way: Open: https://theapacheway.com/open/

Gotcha, good idea, I will add test data generator source code under swift/data-generator/swift-datagen and add a section to the README on how to build it.

I do plan to add the writer for double/bools in the next PR but having the golang test data will be a good way to test the reader impl.

kou · 2023-04-18T21:06:25Z

swift/gen-flatbuffers.sh

+flatc --swift ../../../../format/SparseTensor.fbs
+flatc --swift ../../../../format/Tensor.fbs
+flatc --swift ../../../../format/File.fbs
+popd


Could you prepend our license header to the generated files?

Suggested change

popd

cat <<HEADER > header.swift

// Licensed to the Apache Software Foundation (ASF) under one

// or more contributor license agreements. See the NOTICE file

// distributed with this work for additional information

// regarding copyright ownership. The ASF licenses this file

// to you under the Apache License, Version 2.0 (the

// "License"); you may not use this file except in compliance

// with the License. You may obtain a copy of the License at

//

// http://www.apache.org/licenses/LICENSE-2.0

//

// Unless required by applicable law or agreed to in writing,

// software distributed under the License is distributed on an

// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY

// KIND, either express or implied. See the License for the

// specific language governing permissions and limitations

// under the License.

HEADER

for generated_swift in *_generated.swift; do

mv ${generated_swift} ${generated_swift}.orig

cat header.swift ${generated_swift}.orig > ${generated_swift}

rm ${generated_swift}.orig

done

rm header.swift

popd

kou · 2023-04-18T21:09:02Z

swift/gen-flatbuffers.sh

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+


FYI: We can check "failed to execute a command" and "referred a nonexistent variable" automatically by set -eu (-e is for "failed to execute a command" and -u is for "referred a nonexistent variable"):

Suggested change

set -eu

Will update.

abandy · 2023-04-27T19:46:53Z

Hi @kou, I hope all is well. I believe I have all the changes in. The failed test doesn't seem related to this change. Please review when you get a chance.

kou

Thanks!

I think that we can merge this after we remove swift-datagen and generate it on CI.

kou · 2023-04-28T01:38:13Z

swift/Arrow/README.md

+## Test data generation
+
+Test data files for the reader tests are generated by an executable built in go whose source is included in the data-generator directory.  
+```sh


Suggested change

```sh

```console

Will update.

kou · 2023-04-28T02:00:58Z

swift/Arrow/swift-datagen

Could you remove this from this repository and build it on CI time instead?
I don't want to add auto generated files as much as possible.

Do you know if we will need to add golang to the swift docker image for this?

Yes. I think so.

I think that we can use go run swift-datagen instead of ./swift-datagen in ci/scripts/swift_test.sh.

See also: https://pkg.go.dev/cmd/go

If it's difficult for you, I can help it.

Cool, I have updated the swift image to install golang and updated the script to build swift-datagen.

kou · 2023-04-29T11:53:28Z

swift/data-generator/swift-datagen/go.sum

Could you apply this to accept no-license header for this file?

diff --git a/dev/release/rat_exclude_files.txt b/dev/release/rat_exclude_files.txt index d37790912..f61c21776 100644 --- a/dev/release/rat_exclude_files.txt +++ b/dev/release/rat_exclude_files.txt @@ -149,3 +149,4 @@ r/tools/nixlibs-allowlist.txt .gitattributes ruby/red-arrow/.yardopts .github/pull_request_template.md +swift/data-generator/swift-datagen/go.sum

kou · 2023-04-29T13:34:39Z

swift/Arrow/swift-datagen

Could you remove this file?
Then we can merge this pull request.

Good catch, will do.

kou

+1

Thanks!

ursabot · 2023-04-30T03:53:24Z

Benchmark runs are scheduled for baseline = 16dbd98 and contender = 0ea1a10. 0ea1a10 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️25.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.31% ⬆️0.47%] test-mac-arm
[Failed ⬇️12.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.99% ⬆️0.36%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 0ea1a103 ec2-t3-xlarge-us-east-2
[Failed] 0ea1a103 test-mac-arm
[Failed] 0ea1a103 ursa-i9-9960x
[Finished] 0ea1a103 ursa-thinkcentre-m75q
[Finished] 16dbd98e ec2-t3-xlarge-us-east-2
[Failed] 16dbd98e test-mac-arm
[Failed] 16dbd98e ursa-i9-9960x
[Finished] 16dbd98e 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 · 2023-04-30T04:02:57Z

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

abandy · 2023-05-01T12:13:53Z

Thank you @kou!

- Initial check in for the swift arrow reader impl - bug fixes found during reader testing - class/method access modifier changes (mostly from internal to public) * Closes: apache#34858 Authored-by: Alva Bandy <abandy@live.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added the awaiting review Awaiting review label Apr 2, 2023

abandy changed the title ~~[Swift]Initial arrow reader impl~~ GH-34858: [SWIFT] Initial arrow reader impl Apr 3, 2023

abandy force-pushed the swift-reader-impl branch from 26cab61 to 92900b1 Compare April 3, 2023 13:16

kou changed the title ~~GH-34858: [SWIFT] Initial arrow reader impl~~ GH-34858: [Swift] Initial reader impl Apr 3, 2023

kou reviewed Apr 3, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 4, 2023

abandy force-pushed the swift-reader-impl branch 2 times, most recently from 99f657a to c65eb5a Compare April 5, 2023 02:23

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 5, 2023

abandy force-pushed the swift-reader-impl branch from c65eb5a to 7fbb4d8 Compare April 8, 2023 01:54

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 8, 2023

abandy force-pushed the swift-reader-impl branch from 7fbb4d8 to 8e2fcf1 Compare April 8, 2023 21:38

abandy requested review from assignUser and raulcd as code owners April 8, 2023 21:38

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 8, 2023

abandy force-pushed the swift-reader-impl branch 3 times, most recently from 0040eed to 841978a Compare April 12, 2023 12:47

kou reviewed Apr 18, 2023

View reviewed changes

github-actions bot removed the awaiting change review Awaiting change review label Apr 18, 2023

kou reviewed Apr 28, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 28, 2023

abandy force-pushed the swift-reader-impl branch from 2f660fb to f3b8ba9 Compare April 29, 2023 11:46

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 29, 2023

kou reviewed Apr 29, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 29, 2023

abandy force-pushed the swift-reader-impl branch from f3b8ba9 to 4443b85 Compare April 29, 2023 12:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 29, 2023

kou reviewed Apr 29, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 29, 2023

[Swift]Initial arrow reader impl

d8d1592

abandy force-pushed the swift-reader-impl branch from 4443b85 to d8d1592 Compare April 29, 2023 14:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 29, 2023

kou approved these changes Apr 30, 2023

View reviewed changes

kou merged commit 0ea1a10 into apache:main Apr 30, 2023
45 of 48 checks passed

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 30, 2023

-popd
+cat <<HEADER > header.swift
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+HEADER
+for generated_swift in *_generated.swift; do
+  mv ${generated_swift} ${generated_swift}.orig
+  cat header.swift ${generated_swift}.orig > ${generated_swift}
+  rm ${generated_swift}.orig
+done
+rm header.swift
+popd

GH-34858: [Swift] Initial reader impl #34842

GH-34858: [Swift] Initial reader impl #34842

Conversation

abandy commented Apr 2, 2023 • edited

github-actions bot commented Apr 2, 2023

github-actions bot commented Apr 3, 2023

github-actions bot commented Apr 3, 2023

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 Apr 5, 2023 • edited

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 Apr 18, 2023 • edited

Choose a reason for hiding this comment

abandy Apr 18, 2023 • edited

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 Apr 27, 2023

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

kou left a comment

Choose a reason for hiding this comment

ursabot commented Apr 30, 2023

ursabot commented Apr 30, 2023

abandy commented May 1, 2023

abandy commented Apr 2, 2023 •

edited

abandy Apr 5, 2023 •

edited

abandy commented Apr 18, 2023 •

edited

abandy Apr 18, 2023 •

edited