Copilot commented on code in PR #152:
URL: https://github.com/apache/arrow-swift/pull/152#discussion_r3007470294
##########
Sources/Arrow/ArrowData.swift:
##########
@@ -26,9 +26,26 @@ public class ArrowData {
public let stride: Int
convenience init(_ arrowType: ArrowType, buffers: [ArrowBuffer],
nullCount: UInt) throws {
+ let arrayLength: UInt
+ switch arrowType.info {
+ case .variableInfo:
+ guard buffers.count >= 2 else {
+ throw ArrowError.invalid(
+ "Variable-width ArrowData requires at least two buffers
(null bitmap and offsets).")
Review Comment:
For `.variableInfo`, this convenience init only validates `buffers.count >=
2`, but the rest of the codebase (e.g. `StringArray`/`BinaryArray`) assumes the
`[nullBitmap, offsets, values]` layout and will access `buffers[2]`. Allowing
only 2 buffers can create an `ArrowData` that crashes later at runtime.
Consider requiring `buffers.count >= 3` for variable-width types (or otherwise
validating that the values buffer exists).
```suggestion
guard buffers.count >= 3 else {
throw ArrowError.invalid(
"Variable-width ArrowData requires at least three
buffers (null bitmap, offsets, and values).")
```
##########
Sources/Arrow/ArrowReader.swift:
##########
@@ -195,9 +195,19 @@ public class ArrowReader { // swiftlint:disable:this
type_body_length
let arrowNullBuffer = makeBuffer(nullBuffer, fileData:
loadInfo.fileData,
length: nullLength, messageOffset:
loadInfo.messageOffset)
let arrowOffsetBuffer = makeBuffer(offsetBuffer, fileData:
loadInfo.fileData,
- length: UInt(node.length),
messageOffset: loadInfo.messageOffset)
+ length: UInt(node.length + 1),
messageOffset: loadInfo.messageOffset)
+ let lastOffset = arrowOffsetBuffer.rawPointer
+ .advanced(by: Int(node.length) * MemoryLayout<Int32>.stride)
+ .load(as: Int32.self)
+ guard lastOffset >= 0 else {
+ return .failure(.invalid("Negative last offset (\(lastOffset)) in
variable-width buffer"))
+ }
+ guard lastOffset <= valueBuffer.length else {
Review Comment:
`loadVariableData` compares `lastOffset` (Int32) to `valueBuffer.length`
(Int64). As written (`lastOffset <= valueBuffer.length`) this won’t compile in
Swift due to mismatched integer types. Convert `lastOffset` to `Int64` (or
`valueBuffer.length` to `Int32` after range-checking) before comparing.
```suggestion
guard Int64(lastOffset) <= valueBuffer.length else {
```
--
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]
