From 7da9096f849cf9e81bafd0a1204c6b229def344f Mon Sep 17 00:00:00 2001 From: demetrius albuquerque Date: Sat, 25 Jul 2026 12:12:00 +0200 Subject: [PATCH] fix(ArrowBuilder): correct null bit checking logic and add tests - Fix `isNull` logic in `BaseBufferBuilder`: return true when the null bit is *not* set, instead of checking `nulls.length == 0` or the bit being set. This correctly identifies null values. - Remove redundant `isNull` override in `ListBufferBuilder`, as the base implementation now works correctly. - Add comprehensive tests for `isNull` behavior across all builder types: - FixedBufferBuilder (Int32, Double, Int8, UInt64) - BoolBufferBuilder - VariableBufferBuilder - Scenarios: all nulls, no nulls, mixed, multiple nulls - Add `.vscode/` to .gitignore. This fixes a bug where null values could be incorrectly reported as non-null when the nulls buffer was empty, and ensures consistent behavior across all Arrow buffer builders. --- .gitignore | 2 +- Sources/Arrow/ArrowBufferBuilder.swift | 6 +- Tests/ArrowTests/ArrayBuilderTest.swift | 94 +++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 65bfb48..865f121 100644 --- a/.gitignore +++ b/.gitignore @@ -23,7 +23,7 @@ DerivedData/ Packages/ xcuserdata/ - +.vscode/ # Docker /.docker/ diff --git a/Sources/Arrow/ArrowBufferBuilder.swift b/Sources/Arrow/ArrowBufferBuilder.swift index 4e518c6..e6a980d 100644 --- a/Sources/Arrow/ArrowBufferBuilder.swift +++ b/Sources/Arrow/ArrowBufferBuilder.swift @@ -42,7 +42,7 @@ public class BaseBufferBuilder { } public func isNull(_ index: UInt) -> Bool { - return self.nulls.length == 0 || BitUtility.isSet(index + self.offset, buffer: self.nulls) + return !BitUtility.isSet(index + self.offset, buffer: self.nulls) } func resizeLength(_ data: ArrowBuffer, len: UInt = 0) -> UInt { @@ -429,10 +429,6 @@ public class ListBufferBuilder: BaseBufferBuilder, ArrowBufferBuilder { } } - public override func isNull(_ index: UInt) -> Bool { - return !BitUtility.isSet(index + self.offset, buffer: self.nulls) - } - public func resize(_ length: UInt) { if length > self.offsets.length { let resizeLength = resizeLength(self.offsets) diff --git a/Tests/ArrowTests/ArrayBuilderTest.swift b/Tests/ArrowTests/ArrayBuilderTest.swift index 42e167f..a350c88 100644 --- a/Tests/ArrowTests/ArrayBuilderTest.swift +++ b/Tests/ArrowTests/ArrayBuilderTest.swift @@ -82,4 +82,98 @@ final class ArrayBuilderTests: XCTestCase { XCTAssertThrowsError(try ArrowArrayBuilders.loadBuilder(Int?.self)) XCTAssertThrowsError(try ArrowArrayBuilders.loadBuilder(UInt?.self)) } + + func testFixedBufferBuilderIsNull() throws { + let builder: FixedBufferBuilder = try FixedBufferBuilder() + builder.append(42) + builder.append(nil) + builder.append(7) + + XCTAssertFalse(builder.isNull(0), "Value 42 should not be null") + XCTAssertTrue(builder.isNull(1), "Nil value should be null") + XCTAssertFalse(builder.isNull(2), "Value 7 should not be null") + XCTAssertEqual(builder.nullCount, 1) + } + + func testBoolBufferBuilderIsNull() throws { + let builder = try BoolBufferBuilder() + builder.append(true) + builder.append(nil) + builder.append(false) + + XCTAssertFalse(builder.isNull(0), "Value true should not be null") + XCTAssertTrue(builder.isNull(1), "Nil value should be null") + XCTAssertFalse(builder.isNull(2), "Value false should not be null") + XCTAssertEqual(builder.nullCount, 1) + } + + func testVariableBufferBuilderIsNull() throws { + let builder = try VariableBufferBuilder() + builder.append("hello") + builder.append(nil) + builder.append("world") + + XCTAssertFalse(builder.isNull(0), "String 'hello' should not be null") + XCTAssertTrue(builder.isNull(1), "Nil value should be null") + XCTAssertFalse(builder.isNull(2), "String 'world' should not be null") + XCTAssertEqual(builder.nullCount, 1) + } + + func testFixedBufferBuilderIsNullMultiple() throws { + let builder: FixedBufferBuilder = try FixedBufferBuilder() + builder.append(1.5) + builder.append(nil) + builder.append(2.5) + builder.append(nil) + builder.append(3.5) + + XCTAssertFalse(builder.isNull(0)) + XCTAssertTrue(builder.isNull(1)) + XCTAssertFalse(builder.isNull(2)) + XCTAssertTrue(builder.isNull(3)) + XCTAssertFalse(builder.isNull(4)) + XCTAssertEqual(builder.nullCount, 2) + } + + func testBoolBufferBuilderIsNullMultiple() throws { + let builder = try BoolBufferBuilder() + builder.append(true) + builder.append(true) + builder.append(nil) + builder.append(false) + builder.append(nil) + builder.append(false) + + XCTAssertFalse(builder.isNull(0)) + XCTAssertFalse(builder.isNull(1)) + XCTAssertTrue(builder.isNull(2)) + XCTAssertFalse(builder.isNull(3)) + XCTAssertTrue(builder.isNull(4)) + XCTAssertFalse(builder.isNull(5)) + XCTAssertEqual(builder.nullCount, 2) + } + + func testFixedBufferBuilderIsNullAllNulls() throws { + let builder: FixedBufferBuilder = try FixedBufferBuilder() + builder.append(nil) + builder.append(nil) + builder.append(nil) + + XCTAssertTrue(builder.isNull(0)) + XCTAssertTrue(builder.isNull(1)) + XCTAssertTrue(builder.isNull(2)) + XCTAssertEqual(builder.nullCount, 3) + } + + func testFixedBufferBuilderIsNullNoNulls() throws { + let builder: FixedBufferBuilder = try FixedBufferBuilder() + builder.append(100) + builder.append(200) + builder.append(300) + + XCTAssertFalse(builder.isNull(0)) + XCTAssertFalse(builder.isNull(1)) + XCTAssertFalse(builder.isNull(2)) + XCTAssertEqual(builder.nullCount, 0) + } }