You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "abandy (via GitHub)" <gi...@apache.org> on 2024/07/06 22:08:23 UTC
[PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
abandy opened a new pull request, #43171:
URL: https://github.com/apache/arrow/pull/43171
### Rationale for this change
The struct type requires additions/modifications to builders for buffers and arrays. This is needed in order match functionality with the other supported types.
### What changes are included in this PR?
Updates to buffer and array builders for Struct type.
### Are these changes tested?
Yes, Array test has been added for the Struct type using the Struct builders
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43171:
URL: https://github.com/apache/arrow/pull/43171#issuecomment-2236008600
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0bae073d1abe439d113cc12e06e7ada886a2f2fd.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/27605820751) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1670454335
##########
swift/Arrow/Sources/Arrow/ArrowBufferBuilder.swift:
##########
@@ -327,3 +335,47 @@ public class Date64BufferBuilder: AbstractWrapperBufferBuilder<Date, Int64> {
}
}
}
+
+public final class StructBufferBuilder: BaseBufferBuilder, ArrowBufferBuilder {
+ public typealias ItemType = [Any?]
+ var info: ArrowNestedType?
+ public init() throws {
+ let nulls = ArrowBuffer.createBuffer(0, size: UInt(MemoryLayout<UInt8>.stride))
+ super.init(nulls)
+ }
+
+ public func initializeTypeInfo(_ fields: [ArrowField]) {
+ info = ArrowNestedType(ArrowType.ArrowStruct, fields: fields)
+ }
+
+ public func append(_ newValue: [Any?]?) {
+ let index = UInt(self.length)
+ self.length += 1
+ if length > self.nulls.length {
Review Comment:
Good point, I can remove it but this is just to prevent calling resize for every append call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1673257534
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
Could you add `**This PR includes breaking changes to public APIs.**` to the PR description?
See also our PR template: https://raw.githubusercontent.com/apache/arrow/main/.github/pull_request_template.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #43171:
URL: https://github.com/apache/arrow/pull/43171
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1671462654
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
If we need to call `initialize()` for all `ArrowArray`, I like the `init() throws` approach. Because users can't forget to call `initialize()` with the `init() throws` approach.
In general, do Swift users call `ArrowArray.init()` directory? Or they use builder API to build an array?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1673257534
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
Could you add `**This PR contains a "Critical Fix".**` to the PR description?
See also our PR template: https://raw.githubusercontent.com/apache/arrow/main/.github/pull_request_template.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1670476214
##########
swift/Arrow/Tests/ArrowTests/ArrayTests.swift:
##########
@@ -212,6 +212,61 @@ final class ArrayTests: XCTestCase { // swiftlint:disable:this type_body_length
XCTAssertEqual(microArray[2], 987654321)
}
+ func testStructArray() throws {
+ class StructTest {
+ var field0: Bool = false
+ var field1: Int8 = 0
+ var field2: Int16 = 0
+ var field3: Int32 = 0
+ var field4: Int64 = 0
+ var field5: UInt8 = 0
+ var field6: UInt16 = 0
+ var field7: UInt32 = 0
+ var field8: UInt64 = 0
+ var field9: Double = 0
+ var field10: Float = 0
+ var field11: String = ""
+ var field12 = Data()
+ var field13: Date = Date.now
+ }
+
+ let testData = StructTest()
+ let dateNow = Date.now
+ let structBuilder = try ArrowArrayBuilders.loadStructArrayBuilderForType(testData)
+ structBuilder.append([true, Int8(1), Int16(2), Int32(3), Int64(4),
+ UInt8(5), UInt16(6), UInt32(7), UInt64(8), Double(9.9),
+ Float(10.10), "11", Data("12".utf8), dateNow])
+ structBuilder.append(nil)
+ structBuilder.append([true, Int8(13), Int16(14), Int32(15), Int64(16),
+ UInt8(17), UInt16(18), UInt32(19), UInt64(20), Double(21.21),
+ Float(22.22), "23", Data("24".utf8), dateNow])
+ XCTAssertEqual(structBuilder.length, 3)
+ let structArray = try structBuilder.finish()
+ XCTAssertEqual(structArray.length, 3)
+ XCTAssertNil(structArray[1])
+ XCTAssertEqual(structArray.arrowFields![0].length, 3)
+ XCTAssertNil(structArray.arrowFields![0].array.asAny(1))
+ XCTAssertEqual(structArray[0]![0] as? Bool, true)
+ XCTAssertEqual(structArray[0]![1] as? Int8, 1)
+ XCTAssertEqual(structArray[0]![2] as? Int16, 2)
+ XCTAssertEqual(structArray[0]![3] as? Int32, 3)
+ XCTAssertEqual(structArray[0]![4] as? Int64, 4)
+ XCTAssertEqual(structArray[0]![5] as? UInt8, 5)
+ XCTAssertEqual(structArray[0]![6] as? UInt16, 6)
+ XCTAssertEqual(structArray[0]![7] as? UInt32, 7)
+ XCTAssertEqual(structArray[0]![8] as? UInt64, 8)
+ XCTAssertEqual(structArray[0]![9] as? Double, 9.9)
+ XCTAssertEqual(structArray[0]![10] as? Float, 10.10)
+ XCTAssertEqual(structArray[2]![11] as? String, "23")
+ XCTAssertEqual(String(decoding: (structArray[0]![12] as? Data)!, as: UTF8.self), "12")
Review Comment:
The subscript for the array requires a Int type. I will add an enum with appropriate names to assist with readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1672921377
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
Gotcha, the builders are the only place that should be calling init but since the method is public it can be called by anyone. I will update the init method in ArrowArray to throw. Is there a way to indicate in the PR or issue that this is a breaking change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43171:
URL: https://github.com/apache/arrow/pull/43171#issuecomment-2211986754
:warning: GitHub issue #43168 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on PR #43171:
URL: https://github.com/apache/arrow/pull/43171#issuecomment-2234840788
@kou I hope all is well. I think I have resolved all the issues. Please review again when you get a chance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1670504694
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
Agreed, Adding the throw to init would break compatibility. I added the initialize method to the ArrowArray base class that way it will/should always be called for all ArrowArray. Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43168: [Swift] Add buffer and array builders for Struct type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43171:
URL: https://github.com/apache/arrow/pull/43171#discussion_r1667817830
##########
swift/Arrow/Tests/ArrowTests/ArrayTests.swift:
##########
@@ -212,6 +212,61 @@ final class ArrayTests: XCTestCase { // swiftlint:disable:this type_body_length
XCTAssertEqual(microArray[2], 987654321)
}
+ func testStructArray() throws {
+ class StructTest {
+ var field0: Bool = false
+ var field1: Int8 = 0
+ var field2: Int16 = 0
+ var field3: Int32 = 0
+ var field4: Int64 = 0
+ var field5: UInt8 = 0
+ var field6: UInt16 = 0
+ var field7: UInt32 = 0
+ var field8: UInt64 = 0
+ var field9: Double = 0
+ var field10: Float = 0
+ var field11: String = ""
+ var field12 = Data()
+ var field13: Date = Date.now
+ }
+
+ let testData = StructTest()
+ let dateNow = Date.now
+ let structBuilder = try ArrowArrayBuilders.loadStructArrayBuilderForType(testData)
+ structBuilder.append([true, Int8(1), Int16(2), Int32(3), Int64(4),
+ UInt8(5), UInt16(6), UInt32(7), UInt64(8), Double(9.9),
+ Float(10.10), "11", Data("12".utf8), dateNow])
+ structBuilder.append(nil)
+ structBuilder.append([true, Int8(13), Int16(14), Int32(15), Int64(16),
+ UInt8(17), UInt16(18), UInt32(19), UInt64(20), Double(21.21),
+ Float(22.22), "23", Data("24".utf8), dateNow])
+ XCTAssertEqual(structBuilder.length, 3)
+ let structArray = try structBuilder.finish()
+ XCTAssertEqual(structArray.length, 3)
+ XCTAssertNil(structArray[1])
+ XCTAssertEqual(structArray.arrowFields![0].length, 3)
+ XCTAssertNil(structArray.arrowFields![0].array.asAny(1))
+ XCTAssertEqual(structArray[0]![0] as? Bool, true)
+ XCTAssertEqual(structArray[0]![1] as? Int8, 1)
+ XCTAssertEqual(structArray[0]![2] as? Int16, 2)
+ XCTAssertEqual(structArray[0]![3] as? Int32, 3)
+ XCTAssertEqual(structArray[0]![4] as? Int64, 4)
+ XCTAssertEqual(structArray[0]![5] as? UInt8, 5)
+ XCTAssertEqual(structArray[0]![6] as? UInt16, 6)
+ XCTAssertEqual(structArray[0]![7] as? UInt32, 7)
+ XCTAssertEqual(structArray[0]![8] as? UInt64, 8)
+ XCTAssertEqual(structArray[0]![9] as? Double, 9.9)
+ XCTAssertEqual(structArray[0]![10] as? Float, 10.10)
+ XCTAssertEqual(structArray[2]![11] as? String, "23")
+ XCTAssertEqual(String(decoding: (structArray[0]![12] as? Data)!, as: UTF8.self), "12")
Review Comment:
Can we use field name instead of index such as `structArray[0]!["field0"]` for readability?
##########
swift/Arrow/Sources/Arrow/ArrowArrayBuilder.swift:
##########
@@ -118,6 +118,56 @@ public class Time64ArrayBuilder: ArrowArrayBuilder<FixedBufferBuilder<Time64>, T
}
}
+public class StructArrayBuilder: ArrowArrayBuilder<StructBufferBuilder, StructArray> {
+ let builders: [any ArrowArrayHolderBuilder]
+ let fields: [ArrowField]
+ public init(_ fields: [ArrowField], builders: [any ArrowArrayHolderBuilder]) throws {
Review Comment:
Ah, we can use `throws` for `init()`.
We may want to add `throws` to `ArrowArray.init()` and remove `StructArrowArray.initialize()`. But it breaks compatibility...
##########
swift/Arrow/Sources/Arrow/ArrowBufferBuilder.swift:
##########
@@ -327,3 +335,47 @@ public class Date64BufferBuilder: AbstractWrapperBufferBuilder<Date, Int64> {
}
}
}
+
+public final class StructBufferBuilder: BaseBufferBuilder, ArrowBufferBuilder {
+ public typealias ItemType = [Any?]
+ var info: ArrowNestedType?
+ public init() throws {
+ let nulls = ArrowBuffer.createBuffer(0, size: UInt(MemoryLayout<UInt8>.stride))
+ super.init(nulls)
+ }
+
+ public func initializeTypeInfo(_ fields: [ArrowField]) {
+ info = ArrowNestedType(ArrowType.ArrowStruct, fields: fields)
+ }
+
+ public func append(_ newValue: [Any?]?) {
+ let index = UInt(self.length)
+ self.length += 1
+ if length > self.nulls.length {
Review Comment:
Do we need this check?
`resize()` also has this check.
##########
swift/Arrow/Tests/ArrowTests/ArrayTests.swift:
##########
@@ -212,6 +212,61 @@ final class ArrayTests: XCTestCase { // swiftlint:disable:this type_body_length
XCTAssertEqual(microArray[2], 987654321)
}
+ func testStructArray() throws {
+ class StructTest {
+ var field0: Bool = false
+ var field1: Int8 = 0
+ var field2: Int16 = 0
+ var field3: Int32 = 0
+ var field4: Int64 = 0
+ var field5: UInt8 = 0
+ var field6: UInt16 = 0
+ var field7: UInt32 = 0
+ var field8: UInt64 = 0
+ var field9: Double = 0
+ var field10: Float = 0
+ var field11: String = ""
+ var field12 = Data()
+ var field13: Date = Date.now
Review Comment:
How about using type name instead of index for field name such as `fieldBool`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org