search

tradewelltech / protarrow

Convert from protobuf to arrow and back
https://protarrow.readthedocs.io/
Apache License 2.0
17 stars 2 forks source link

adding presence check for non-message type fields #69

Closed chasezheng closed 6 months ago

chasezheng commented 7 months ago

This is to fix issue#68

chasezheng commented 6 months ago

Hi Michael, do we need to run the workflow in order to merge? I am not authorized to merge.

0x26res commented 6 months ago

Hi @chasezheng, thanks for your contribution it is much appreciated.

At first glance, I think it should work (though I could have forgotten another place where we should check for has_presence.

As @mwong38 said, we would need some tests.

First please read our development doc: https://protarrow.readthedocs.io/en/latest/development/

The simplest way would be for you to add a test message to https://github.com/tradewelltech/protarrow/blob/master/protos/example.proto and in https://github.com/tradewelltech/protarrow/blob/master/tests/test_protobuf.py add a test that goes back and forth from your message to table and make sure nullability is preserved.

Now we also have more systematic tests but it is a bit more involved and I can add them later.

0x26res commented 6 months ago

@chasezheng let us know if there's anything we can help with, or if you want us to take over the MR. Thanks.

chasezheng commented 6 months ago

If you can take over the PR that would be really appreciated. I might be able to take another at weekend but I can't promise.

0x26res commented 6 months ago

OK, I had a god at it here https://github.com/tradewelltech/protarrow/pull/71/

I'm assuming this only works for primitive and enums. I don't think it works for messages (which are optional by default) and repeated/map (never nullable). Is this correct?