Add Cython bindings for arrow::ArrayBuilder classes #2

prashantmital · 2021-02-05T08:15:06Z

Adds wrappers for ArrayBuilder APIs.

In order to run this:

first refer to the description of Cython bindings for libbson #1 .
then, install pyarrow and run python -c "import pyarrow as pa; pa.create_library_symlinks()
to run the tests do python -m unittest test/test_builder.py

TODO

~~Add builders for other supported types~~ - will be added in subsequent PRs
Add install_requires and test_requires sections to setup.py

ShaneHarvey

Is it possible to add a simple github actions setup for the test suite?

ShaneHarvey · 2021-02-05T16:10:57Z

bindings/python/pymongoarrow/builder.pyx

+import numpy as np
+
+
+cdef class Int32Builder(_Weakrefable):


_Weakrefable is this private? Is there a public class we can use instead?

_Weakrefable is c-defined in pyarrow.lib. It just adds the ability for users to weakref this class. Strictly speaking, we don't need it at the moment in this implementation but payarrow's classes are all Weakrefable so I included it here for consistency.

The problem is that we're relying on a private api so let's not use _Weakrefable at all.:

cdef class _Weakrefable: cdef object __weakref__

Adding the __weakref__ attribute allows these Python objects to be used with weakref.ref(). Do we expect to weakref() these classes? If not, then we shouldn't bother. If it turns out we need to support this then we can define _Weakrefable ourselves.

Hmm ok I see your point. Removed.

ShaneHarvey · 2021-02-05T16:13:55Z

bindings/python/setup.py

+        module.libraries.extend(pa.get_libraries())
+        module.library_dirs.extend(pa.get_library_dirs())
+        if os.name == 'posix':
+            module.extra_compile_args.append('-std=c++11')


Could you add a comment linking to https://arrow.apache.org/docs/python/extending.html#example so we know where this pattern comes from?

ShaneHarvey · 2021-02-05T16:20:28Z

bindings/python/setup.py



 setup(
    name='pymongoarrow',
    version=get_pymongoarrow_version(),
    packages=find_packages(),
    ext_modules=get_extension_modules(),
-    setup_requires=['cython >= 0.29'])
+    setup_requires=['cython >= 0.29', 'pyarrow >= 3', 'numpy >= 1.16.6'])


Let me see if I have this right. So we need pyarrow+numpy to build+install the wheel but the wheel can be installed and used even without pyarrow+numpy? Or are we missing a necessary install_requires?

We are missing the install_requires - we will depend on pymongo and pyarrow (pyarrow has a dependency on numpy). Pandas would need to be installed manually if the user intends to use that.

Can we add install_requires=['pyarrow >= 3']?

ShaneHarvey · 2021-02-05T16:23:48Z

bindings/python/test/test_builder.py

+# limitations under the License.
+from unittest import TestCase
+
+from pyarrow import Array, int32, int64


Continuing my comment above, since we use pyarrow in the test suite, should we add it to test_requires or install_requires?

Add a TODO do add these.

If we add arrow to install_requires then we don't need test_requires at all.

So far, this is correct. Eventually, we will likely need pandas for our tests but the package wouldn't really depend on it... We can add a test_requires for pandas when that happens.

ShaneHarvey · 2021-02-05T16:33:17Z

bindings/python/pymongoarrow/builder.pyx

+        return self.builder.get().length()
+
+    cdef shared_ptr[CInt64Builder] unwrap(self):
+        return self.builder


Hmm these classes are so similar. There must be a way to share the implementation, right? I found this blog about it https://martinralbrecht.wordpress.com/2017/07/23/adventures-in-cython-templating/

But for now I don't think we should use any of these "templating" options and instead keep it simple.

Yeah I tried a bunch of different things for sharing code here since so much of it is common and since Cython does support class inheritance. Unfortunately, the scoping rules enforced on cdef-ined variables by Cython are causing problems when I try to share code between these classes. For now I have given up on this front. We can make this more sophisticated after 0.1.0

Update - I did a bit of updating... Some more optimizations will be possible in a subsequent PR.

ShaneHarvey · 2021-02-05T20:55:38Z

bindings/python/pymongoarrow/builder.pyx

+        elif isinstance(value, int):
+            self.builder.get().Append(value)
+        else:
+            raise TypeError('Int32Builder only accepts integer objects')


Instead of calling isinstance(value, int), can we rely on CInt32Builder to raise an error when the value is not supported? If so it would have better performance.

We aren't going to be using this method to actually construct the arrays from BSON streams. That code will be pure-C/C++. These wrapper classes will help us create the necessary builders etc from Python code instead of needing to do this via C++ code which is rather complex due to scoping rules enforced by Cython.

See the unwrap method in all these classes - this will eventually be used by some Cython code to access the underlying builders directly to construct arrays. The need for an unwrap method is also why the underlying builder classes are cdefined as shared_ptr instances instead of unique_ptr instances (so that they can be unwrapped and a new reference to them can be passed to pure-C code somewhere).

prashantmital

I will add a GH actions script in a subsequent commit or PR.

prashantmital · 2021-02-08T21:24:55Z

bindings/python/pymongoarrow/builder.pyx

+import numpy as np
+
+
+cdef class Int32Builder(_Weakrefable):


Hmm ok I see your point. Removed.

prashantmital · 2021-02-08T21:28:35Z

bindings/python/pymongoarrow/builder.pyx

+        elif isinstance(value, int):
+            self.builder.get().Append(value)
+        else:
+            raise TypeError('Int32Builder only accepts integer objects')


We aren't going to be using this method to actually construct the arrays from BSON streams. That code will be pure-C/C++. These wrapper classes will help us create the necessary builders etc from Python code instead of needing to do this via C++ code which is rather complex due to scoping rules enforced by Cython.

See the unwrap method in all these classes - this will eventually be used by some Cython code to access the underlying builders directly to construct arrays. The need for an unwrap method is also why the underlying builder classes are cdefined as shared_ptr instances instead of unique_ptr instances (so that they can be unwrapped and a new reference to them can be passed to pure-C code somewhere).

prashantmital · 2021-02-08T21:29:09Z

bindings/python/pymongoarrow/builder.pyx

+        return self.builder.get().length()
+
+    cdef shared_ptr[CInt64Builder] unwrap(self):
+        return self.builder


Update - I did a bit of updating... Some more optimizations will be possible in a subsequent PR.

prashantmital · 2021-02-08T21:29:14Z

bindings/python/setup.py

+        module.libraries.extend(pa.get_libraries())
+        module.library_dirs.extend(pa.get_library_dirs())
+        if os.name == 'posix':
+            module.extra_compile_args.append('-std=c++11')


prashantmital · 2021-02-08T21:29:25Z

bindings/python/setup.py



 setup(
    name='pymongoarrow',
    version=get_pymongoarrow_version(),
    packages=find_packages(),
    ext_modules=get_extension_modules(),
-    setup_requires=['cython >= 0.29'])
+    setup_requires=['cython >= 0.29', 'pyarrow >= 3', 'numpy >= 1.16.6'])


prashantmital · 2021-02-08T21:30:33Z

bindings/python/test/test_builder.py

+# limitations under the License.
+from unittest import TestCase
+
+from pyarrow import Array, int32, int64


So far, this is correct. Eventually, we will likely need pandas for our tests but the package wouldn't really depend on it... We can add a test_requires for pandas when that happens.

ShaneHarvey · 2021-02-08T22:06:46Z

bindings/python/test/test_builders.py

+
+
+class TestDate64Builder(TestCase):
+    def test_simple(self):


Should we test the 's' unit here as well?

Add Cython bindings for arrow::ArrayBuilder classes

dd468dc

prashantmital requested review from behackett and ShaneHarvey February 5, 2021 08:15

add methods to unwrap classes

37dbf12

ShaneHarvey reviewed Feb 5, 2021

View reviewed changes

add support for more types

9204949

prashantmital commented Feb 8, 2021

View reviewed changes

reorg code

a90ebc8

ShaneHarvey reviewed Feb 8, 2021

View reviewed changes

test seconds

ac9a78e

prashantmital requested a review from ShaneHarvey February 8, 2021 23:29

ShaneHarvey approved these changes Feb 9, 2021

View reviewed changes

prashantmital merged commit dedb1e0 into main Feb 9, 2021

prashantmital deleted the feat/arrow-builders branch February 9, 2021 00:40

Add Cython bindings for arrow::ArrayBuilder classes #2

Add Cython bindings for arrow::ArrayBuilder classes #2

Uh oh!

Conversation

prashantmital commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prashantmital left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prashantmital commented Feb 5, 2021 •

edited

Loading