|
Some features of unions:
I can't really view the semantics and uses of this. If we take the semantics of a C union, wouldn't
union MyUnion { 1: string a_string, 2: i32 an_i32, } be roughly equivalent to struct MyUnion { 1: optional string a_string, 2: optional i32 an_i32, } with exclusion constraints enforced at the app level? Maybe it's not really a union. It's kind of more like a "one-of" struct. I should have pointed out as an additional feature that unlike a true union, you should NOT be able to use this structure to "convert" between one datatype and another. Whatever type it is when you create it, it's that type until you set it to another value.
As such, it may not actually be implemented in c/cpp as a true union. It might need to be class wrapped around a union.
I say "yes and no" to this. Union-aware readers should probably throw an exception if more than one field is set, but I think that we should not create a special wire-format for unions. Just use the struct format.
That doesn't really work in containers, and I think it actually makes the code quite a bit trickier in structs. Maybe we could have a special setting for each union indicating that none of the fields are set.
I think I disagree with this. I think we have to have this type of representation in order to be able to read a union with no set field. Therefore, we might as well give apps the ability to use it.
Do you mean "which of its fields is set"? If so, I agree. Probably just an int equal to the id of the field that was read, which will make it faster than checking isset for each individual field.
Your description of the semantics is exactly correct. In addition, we can make it more efficient to determine which field is set. I'll let Bryan comment on the uses. Also, there is no need to add any new syntax for this while we are experimenting with it. We can just make it an annotation. I think a class wrapped around a boost::variant is more likely. You can't put strings and vectors in unions.
I thought of this as an option, but I think that will lead to suboptimal wire performance. For instance, we definitely do not need a whole extra stop byte at the end of the union just to say there's nothing more coming. There should only ever be one field, so I say let's tailor it accordingly.
I'll admit I'm not sure on how the container thing might work, but I don't think it will make the struct code that much more complicated. I was thinking that our generated union classes would have a static read() method that could return null if an invalid union was read from the wire. If a null union is unacceptable in any circumstance, I think we should just use some constant "invalid" union or something to indicate there was a problem. However, I don't think an empty union should be serializable.
Yep.
Maybe. I think we'd kind of have to do a lot of working around the built in generator behavior to do this, and if we ultimately decide to have this feature, it'd be silly to use annotations for it.
It's one byte. Using a different wire format makes it impossible to implement this incrementally. Every language must be able to skip unions before you can start writing them. Plus it makes it impossible to transparently upgrade from current "all-optional" structures.
I think it is important to figure the containers out, and I disagree about the struct case. If you have a state that represents a union whose content could not be read, then the code for reading a union looks exactly the same as the code for reading a structure, when viewed from the perspective of the caller. I see what you're saying about incremental roll-out. At the cost of one additional byte, it might be worth it to keep the same wire format. To make the code efficient, though, I don't think that a union should include the logic for "skipping" subsequent fields. That is, I don't want to write a loop at all in the read() method, so it might just look like, read field id, read type, read value, read stop, if not stop, throw protocol exception.
Here's my first bit of work on this issue. This patch includes some changes to the lexer and parser to add the "union" construct, some changes to the compiler to support unions, and work on the java library and generator to support generating unions.
There's a small test for the java unions, but it needs to be greatly expanded upon. I think this should probably be done with less code duplication. Specifically:
I'd be open to this change, but I'm not exactly clear on how all the parser stuff works. I did more of a copy and paste approach than a genuine change.
I was sort of going for this, though I can see how it would make sense to refine it a little. My overall intent was to diverge the code generators quite a bit between structs and unions, since I'm expecting to be able to do a lot of simplifying things for unions. (Take a look at the TUnion base class that's included in the patch - there doesn't have to be as much generated code in unions as structs.) Well, the easiest way to parse it would be to just add it as an annotation.
 This patch (on top of yours) should do the trick. It uses add_struct for both structs and unions. You might have to delete trailing whitespace to apply it. What's the status of this? Can we get someone to review and commit?
This issue is still very much work in progress. There isn't even one language that implements union as something other than a struct. We could conceivably commit the general compiler stuff first and work on languages later, but I'm not in any hurry to do that.
Ok, here is an incremental improvement on the patch I put up so long ago. It was a lot further along than I remembered! This now includes David's patch.
I'd love to get some comments on what needs improvement, both from the thrift code and user-facing code standpoint, if people have input to give. You added some whitespace to a blank line at the end of the parser (.yy). Also, I think a lot of the stuff you did under src/parse can be reverted since my version of the parser change just treats unions as structs with a special flag set. I haven't looked at the Java-specific stuff yet.
OK, I removed some of the unnecessary parser changes. I also reorganized some of the generated code, and generated a field metadata map so that it looks like other TBase objects.
Here's the latest patch that includes a compareTo for compatible unions.
I read through thrift-409-v4.patch and saw a couple pointer versus value issues:
187: value_ = other.value_ The todo on 237 is probably worth doing, since a default constructor is declared. This patch addresses David's issues, and as a side effect cleans up some confusing stuff in TBase.java.
This fixes a bug in the equals method and switches all the shorts to ints.
This version includes some minor bug fixes, as well as helper "constructors" so that you can instantiate a union of a given field type in a java-type-safe manner.
I would love for some more reviews of this code. I think it's getting pretty darn close to ready to commit. Rapleaf is already gearing up to make use of it in near-production. Anyone have time to take a look?
In the Java-specific stuff, things seem a bit too type-unsafe to me. A lot of it has to do with limitations inherent to Java generics, but in some places type info is lost where it mustn't. For instance, the deep copy constructor uses generic code in TUnion.java instead of custom generated code like a normal struct would do. The constructor could instead check for a valid value of setField_, and invoke the corresponding (generated) type-safe code (I see there is a checkType() method, but it doesn't work for containers).
Likewise, instead of having just one constructor like this: TUnion(int setField, Object value) there could be one with an explicit type for each field, with the corresponding check that the appropriate value for setField is used according to the field's type. This way user code attempting to assign an object of invalid type to a field would not even compile. The generic constructor is intentionally type-unsafe for flexibility. If you want type safety, you can use the FooUnion.field_name() style static helper constructors. Those are type safe and guarantee that you are getting the field id you expected.
I made the choice not to use a ton of generated code in TUnion because it greatly simplifies the generator and makes the code much easier to follow.
I would imagine most of the required code is already there in the generator since normal structs use it. this patch only adds actual union code generation for Java, right?
That's correct. It's intended to serve as a reference implementation for other language developers. As a bonus, languages that don't yet generate unions specifically can just generate a struct, and everything will continue to work.
Updated to work with the latest trunk.
Does anyone object to me committing this? It has been available for comment for quite some time, and there aren't any real outstanding bugs in the implementation. I believe it is ready to be used, and that improvements can be made in separate tickets after this initial issue is committed. If no one chimes in to the contrary, I will commit this patch tomorrow evening.
Undo changes to t_type.h, and this hunk:
Unions are a subset of structures, so there is no need to have t_type be aware of their existence, or check for them specially. Should deepCopyObject be used for TStruct as well? + protected TUnion(int setField, Object value) { It looks like read doesn't verify that the on-the-wire type is the same as the type that is expected. All of the struct implementations skip the field when this is the case. Unless I'm missing something, the biggest problem is that you do not call writeFieldStop in write. This means that a union-unaware recipient would be unable to skip over a union that was sent to them, and their deserialization would desynchronize. I think this is a blocker. See https://issues.apache.org/jira/browse/THRIFT-409?focusedCommentId=12689715&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12689715 Never mind about verifying on-the-wire types. I see it now. Also, you are missing a newline in the gen-code at the end of the single-field named constructors.
Printing of byte[] in Java implementation is broken. It prints as [B@6e0142c.
Wow, yeah, this is definitely an oversight on my part. Is it just me or is writeFieldStop actually misnamed? Isn't it really the end of the struct, not a field? It is a special field header that signifies the end of the fields. Misnamed? No. Unintuitive? Yes.
I think the simplest test for the skip functionality is to write your StructWithAUnion to a buffer, then try to read that buffer as a "Emtpy" (defined in the same .thrift file).
OK, this patch fixes the problems mentioned in the comments. Can you let me know if this looks ok? I included the mentioned Empty skip test, which passes now.
This seems good. I'm pretty sure there is one more bug, but it is not a blocker for me if it is not a blocker for you. As you are aware, you don't verify that that the second field header is actually the stop field. As a result, if someone sends you a struct with more than one field (like, a C++ client that has non-nullable fields and currently ignores the union/struct distinction) that you expect to be a union, you will desynchronize. I think the simplest fix would be to check it and throw an exception if it is not the stop field. You could also
There is probably a similar problem if you attempt to deserialize a struct with no fields. sigh. You could also just skip the rest of the fields and say the first one wins. Or the last one wins. Or a random one wins, to make sure we don't depend on coincidental but repeatable behavior. Seriously, though, it is probably best to check and throw.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
New Feature
Resolved
Major
Add "union" to Thrift
union <union name> { <field id>: <type> <field name> <optional line ender> }An example would look like:
union MyUnion { 1: string a_string; 2: i32 an_i32; }