kou commented on code in PR #33:
URL: https://github.com/apache/arrow-swift/pull/33#discussion_r2147337051
##########
Arrow/Sources/Arrow/ArrowType.swift:
##########
@@ -366,6 +417,24 @@ public class ArrowType {
return ArrowTypeTime64(.microseconds)
} else if from == "ttn" {
return ArrowTypeTime64(.nanoseconds)
+ } else if from.starts(with: "ts") {
+ let components = from.split(separator: ":", maxSplits: 1)
+ guard let unitPart = components.first, unitPart.count == 3 else {
+ throw ArrowError.notImplemented
Review Comment:
It seems that this is an invalid case like `tsXX`. Can we use more suitable
exception than `ArrowError.notImplemented`?
##########
Arrow/Sources/Arrow/ProtoUtil.swift:
##########
@@ -64,6 +64,22 @@ func fromProto( // swiftlint:disable:this
cyclomatic_complexity function_body_le
let arrowUnit: ArrowTime64Unit = timeType.unit == .microsecond ?
.microseconds : .nanoseconds
arrowType = ArrowTypeTime64(arrowUnit)
}
+ case .timestamp:
+ let timestampType = field.type(type:
org_apache_arrow_flatbuf_Timestamp.self)!
+ let arrowUnit: ArrowTimestampUnit
+ switch timestampType.unit {
+ case .second:
+ arrowUnit = .seconds
+ case .millisecond:
+ arrowUnit = .milliseconds
+ case .microsecond:
+ arrowUnit = .microseconds
+ case .nanosecond:
+ arrowUnit = .nanoseconds
+ }
+
+ let timezone = timestampType.timezone
+ arrowType = ArrowTypeTimestamp(arrowUnit, timezone: timezone?.isEmpty
== true ? nil : timezone)
Review Comment:
Can we omit `== true` here?
```suggestion
arrowType = ArrowTypeTimestamp(arrowUnit, timezone:
timezone?.isEmpty ? nil : timezone)
```
##########
Arrow/Tests/ArrowTests/CDataTests.swift:
##########
@@ -43,7 +43,11 @@ final class CDataTests: XCTestCase {
.addField("colTime64", type: ArrowType(ArrowType.ArrowTime64),
isNullable: false)
.addField("colTime64u", type: ArrowTypeTime64(.microseconds),
isNullable: false)
.addField("colTime64n", type: ArrowTypeTime64(.nanoseconds),
isNullable: false)
- .addField("colTime64", type: ArrowType(ArrowType.ArrowTime64),
isNullable: false)
+ .addField("colTimestamp", type:
ArrowType(ArrowType.ArrowTimestamp), isNullable: false)
+ .addField("colTimestamptsn", type:
ArrowTypeTimestamp(.nanoseconds), isNullable: false)
+ .addField("colTimestamptsu", type:
ArrowTypeTimestamp(.microseconds), isNullable: false)
+ .addField("colTimestamptsm", type:
ArrowTypeTimestamp(.milliseconds), isNullable: false)
+ .addField("colTimestamptss", type: ArrowTypeTimestamp(.seconds),
isNullable: false)
Review Comment:
* It seems that `s` is redundant
* Should we use `seconds` -> `milliseconds` -> `microseconds` ->
`nanoseconds` order like `Time{32,64}`?
```suggestion
.addField("colTimestampts", type: ArrowTypeTimestamp(.seconds),
isNullable: false)
.addField("colTimestamptm", type:
ArrowTypeTimestamp(.milliseconds), isNullable: false)
.addField("colTimestamptu", type:
ArrowTypeTimestamp(.microseconds), isNullable: false)
.addField("colTimestamptn", type:
ArrowTypeTimestamp(.nanoseconds), isNullable: false)
```
##########
Arrow/Sources/Arrow/ArrowType.swift:
##########
@@ -366,6 +417,24 @@ public class ArrowType {
return ArrowTypeTime64(.microseconds)
} else if from == "ttn" {
return ArrowTypeTime64(.nanoseconds)
+ } else if from.starts(with: "ts") {
+ let components = from.split(separator: ":", maxSplits: 1)
+ guard let unitPart = components.first, unitPart.count == 3 else {
+ throw ArrowError.notImplemented
+ }
+
+ let unitChar = unitPart.suffix(1)
+ let unit: ArrowTimestampUnit
+ switch unitChar {
+ case "s": unit = .seconds
+ case "m": unit = .milliseconds
+ case "u": unit = .microseconds
+ case "n": unit = .nanoseconds
+ default: unit = .milliseconds
Review Comment:
Should we raise an exception here?
This case is for `tsX`, right?
##########
Arrow/Tests/ArrowTests/ArrayTests.swift:
##########
@@ -212,6 +212,76 @@ final class ArrayTests: XCTestCase { //
swiftlint:disable:this type_body_length
XCTAssertEqual(microArray[2], 987654321)
}
+ func testTimestampArray() throws {
+ // Test timestamp with seconds unit
+ let secBuilder = try
ArrowArrayBuilders.loadTimestampArrayBuilder(.seconds, timezone: nil)
+ secBuilder.append(1609459200) // 2021-01-01 00:00:00
+ secBuilder.append(1609545600) // 2021-01-02 00:00:00
+ secBuilder.append(nil)
+ XCTAssertEqual(secBuilder.nullCount, 1)
+ XCTAssertEqual(secBuilder.length, 3)
+ XCTAssertEqual(secBuilder.capacity, 264)
+ let secArray = try secBuilder.finish()
+ let secType = secArray.arrowData.type as! ArrowTypeTimestamp //
swiftlint:disable:this force_cast
+ XCTAssertEqual(secType.unit, .seconds)
+ XCTAssertNil(secType.timezone)
+ XCTAssertEqual(secArray.length, 3)
+ XCTAssertEqual(secArray[0], 1609459200)
+ XCTAssertEqual(secArray[1], 1609545600)
+ XCTAssertNil(secArray[2])
+
+ // Test timestamp with milliseconds unit and timezone America/New_York
+ let msBuilder = try
ArrowArrayBuilders.loadTimestampArrayBuilder(.milliseconds, timezone:
"America/New_York")
+ msBuilder.append(1609459200000) // 2021-01-01 00:00:00.000
+ msBuilder.append(nil)
+ msBuilder.append(1609545600000) // 2021-01-02 00:00:00.000
+ XCTAssertEqual(msBuilder.nullCount, 1)
+ XCTAssertEqual(msBuilder.length, 3)
+ XCTAssertEqual(msBuilder.capacity, 264)
+ let msArray = try msBuilder.finish()
+ let msType = msArray.arrowData.type as! ArrowTypeTimestamp //
swiftlint:disable:this force_cast
+ XCTAssertEqual(msType.unit, .milliseconds)
+ XCTAssertEqual(msType.timezone, "America/New_York")
+ XCTAssertEqual(msArray.length, 3)
+ XCTAssertEqual(msArray[0], 1609459200000)
+ XCTAssertNil(msArray[1])
+ XCTAssertEqual(msArray[2], 1609545600000)
+
+ // Test timestamp with microseconds unit and timezone America/New_York
Review Comment:
```suggestion
// Test timestamp with microseconds unit and timezone UTC
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
