search

appnexus / pyrobuf

A Cython alternative to Google's Python Protobuf library
Other
554 stars 76 forks source link

Implement `oneof` codegen #147

Closed thirtythreeforty closed 4 years ago

thirtythreeforty commented 4 years ago

This pull request adds support for code generation of oneof clauses. The heavy lifting has already been done by #122; this simply supports them on the template backend. Since the parser already emits the fields into the enclosing message, this MR simply clears sibling fields in the setters for each oneof field.

The trickiest part is handling the semantics of submessages in oneof clauses - since a "default" proxy object is returned like in Google's library, the clearing of sibling fields must be delayed until that proxy is actually modified. (Google's design choice here is really weird in my opinion.) I introduce new _ModifiedOneof_* methods here as listeners, refactoring the "listener" mechanism to provide an arbitrary callable. There is a speed hit on deserialization here:

Old (s) New (s)
serialize 0.394717 0.395081
deserialize 0.263785 0.347662
HasField 0.013982 0.009290

For my use case, I'm happy to take a small hit here in exchange for actually getting to use Pyrobuf in my application, but I'm open to suggestions to re-speed this up.

There remains a problem with the parser: it cannot handle oneof blocks in proto2 syntax (it is expecting a required or optional qualifier for each field, which is not valid inside a oneof block). But since I don't need proto2, I'm leaving this for someone else to tackle. I'm pretty sure it is an easy fix. Accordingly, there is also a patch to actually run unit tests on proto3 files, which is not happening in master.

Resolves #95. Please review patch-wise for greatest clarity.

thirtythreeforty commented 4 years ago

It also seems that __str__ does not respect whether a message field is present - printing a oneof prints all its (defaulted) variants. I'm not sure if that should be a separate PR or not.

tburmeister commented 4 years ago

Sorry I am just getting around to looking at this. Overall, looks great; thanks for doing this! I think it's fine if this is unsupported for proto2. With regards to the __str__ issue, should be able to track which value is actually set, right? But I also think you could make an argument that the current behavior is "correct", so happy to punt on this to a future PR.

thirtythreeforty commented 4 years ago

I think the str behavior is irritating and different from that of Google's Protobuf library, but also outside the scope of this PR. But I can be swayed either way.

ahobsonsayers commented 4 years ago

I just wanted to say, this PR looks great, I am in need of the oneof feature. Is there any blockers to this being merged? I could help if needs be.

thirtythreeforty commented 4 years ago

@ahobsonsayers I have recently changed jobs so this code isn't a priority for me right now, as much as I hate to leave something half-finished. If you want to help that would be great.

tburmeister commented 4 years ago

There were just a few things I wanted to have cleaned up before merging. I can take a look this weekend and try to finish this up.

ahobsonsayers commented 4 years ago

No worries @thirtythreeforty, thanks for all the work you did on it up to this point. I've been using your branch temporarily until this is merged, and it seems to be working really well so thanks for that.

@tburmeister, that sounds great. Let me know if you need any help, i can put some time in if needs be.

tburmeister commented 4 years ago

Made some additional changes which should hopefully make things a bit cleaner as well. In particular, added per-field reset methods.

tburmeister commented 4 years ago

@ahobsonsayers @thirtythreeforty this is now available in pyrobuf-0.9.2. Thanks @thirtythreeforty !