apache / arrow-nanoarrow

Helpers for Arrow C Data & Arrow C Stream interfaces
https://arrow.apache.org/nanoarrow
Apache License 2.0
149 stars 34 forks source link

fix(Python): Ensure we don't call cuMemAlloc with 0 bytesize #534

Closed shwina closed 1 week ago

shwina commented 1 week ago

According to the CUDA docs for cuMemAlloc:

If bytesize is 0, cuMemAlloc() returns CUDA_ERROR_INVALID_VALUE.

We end up calling cuMemAlloc() with 0 bytesize when allocating device buffers with no null mask. Thus, the following change was causing nanoarrow_device_cuda_test to fail:

@@ -207,7 +207,8 @@ TEST_P(StringTypeParameterizedTestFixture, ArrowDeviceCudaArrayViewString) {
   ASSERT_EQ(ArrowArrayStartAppending(&array), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("abc")), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("defg")), NANOARROW_OK);
-  ASSERT_EQ(ArrowArrayAppendNull(&array, 1), NANOARROW_OK);
+  ASSERT_EQ(ArrowArrayAppendString(&array, ArrowCharView("defg")), NANOARROW_OK);
+  // ASSERT_EQ(ArrowArrayAppendNull(&array, 1), NANOARROW_OK);
   ASSERT_EQ(ArrowArrayFinishBuildingDefault(&array, nullptr), NANOARROW_OK);

   ASSERT_EQ(ArrowDeviceArrayInit(cpu, &device_array, &array, nullptr), NANOARROW_OK);

In this PR, I've fixed this by simply skipping the call to cuMemAlloc. The resulting buffer will have nullptr as its data member and 0 as its size_bytes, which I believe is the desired outcome.

I also modified the test above to include cases with no nulls.

shwina commented 1 week ago

I see that this logic is being overhauled in https://github.com/apache/arrow-nanoarrow/pull/509/. If the maintainers want to close this PR out, that's fine by me!

It's probably worth including the changes to the tests though.