[jira] [Comment Edited] (THRIFT-2232) IsSet* broken in Go

["Ben Sigelman (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-2232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13801855#comment-13801855 ] 

Ben Sigelman edited comment on THRIFT-2232 at 10/22/13 4:29 PM:
----------------------------------------------------------------

Hi [~jensg], thanks for the quick response!

There are, of course, a variety of options. Here are some things I'd like to maintain:
 - allow users to continue to treat thrift structs as simple Go structs, at least to a first approximation
 - do not force users to manually maintain a bit vector of IsSet information
 - backwards compatibility would be awesome

The sad thing is that I can't imagine a solution that accomplishes all of the above. Since these are still relatively early days (and since the current library is broken per this bug report), I am inclined to give up the backwards compatibility goal.

One option (which I do not favor) would be to generate Go interfaces rather than Go structs. These interfaces would be the only way to get/set fields, and could internally do any number of things to satisfy the various method guarantees. The actual implementation of the thrift-generated interface could take many forms, but it definitely wouldn't be exported from the generated package. I don't like this idea, though, as it seems a little heavy-weight and replaces simple struct accessors with method calls. It would be great to allow users to treat generated thrift structs as Go structs somehow.

My proposal is to do something akin to what protobufs does to solve this problem. (It's also similar to what other popular packages do for things like this... e.g., the way that the `gorp` ORM package deals with nullable database columns) In particular, we would represent an optional field as a pointer, and use the `nil` value to represent absence. Required fields could either (a) also be represented as pointers and make their containing structs invalid when absent, or (b) be represented as non-pointer fields like the current thrift Go impl does for all fields. list<>s etc would hopefully be unaffected by this proposal... they can still be empty, and that's all we need as far as presence/absence info (correct me if I'm mistaken!).

Also, note that it's annoying [though still totally feasible] to pass pointers to POD and string literals in Go. Protobufs deals with this by providing helpers. We would probably do the same... See these guys:

http://godoc.org/code.google.com/p/goprotobuf/proto#String
http://godoc.org/code.google.com/p/goprotobuf/proto#Uint32

The above begs the question of whether there can/should be a central lib of Go thrift helpers that people can add to their projects (rather than simply embedding all of these, redundantly, in every generated thrift Go package).

So, let me know what you think. FYI, I know a lot about protobufs (was at google for 9 years), but less about Thrift per se. I'd love to know if there are thorny things I should be aware of in advance.

Also, if there's someone else who's willing to do this work, I have a million other things to do and would gladly wait for them if the turnaround would be less than a month or so. Thanks for your help!


was (Author: bensigelman):
Hi [~jensg], thanks for the quick response!

There are, of course, a variety of options. Here are some things I'd like to maintain:
 - allow users to continue to treat thrift structs as simple Go structs, at least to a first approximation
 - do not force users to manually maintain a bit vector of IsSet information
 - backwards compatibility would be awesome

The sad thing is that I can't imagine a solution that accomplishes all of the above. Since these are still relatively early days (and since the current library is broken per this bug report), I am inclined to give up the backwards compatibility goal.

One option (which I do not favor) would be to generate Go interfaces rather than Go structs. These interfaces would be the only way to get/set fields, and could internally do any number of things to satisfy the various method guarantees. The actual implementation of the thrift-generated interface could take many forms, but it definitely wouldn't be exported from the generated package. I don't like this idea, though, as it seems a little heavy-weight and replaces simple struct accessors with method calls. It would be great to allow users to treat generated thrift structs as Go structs somehow.

My proposal is to do something akin to what protobufs does to solve this problem. (It's also similar to what other popular packages do for things like this... e.g., the way that the `gorp` ORM package deals with nullable database columns) In particular, we would represent an optional field as a pointer, and use the `nil` value to represent absence. Required fields could either (a) also be represented as pointers and make their containing structs invalid when absent, or (b) be represented as non-pointer fields like the current thrift Go impl does for all fields. list<>s etc would hopefully be unaffected by this proposal... they can still be empty, and that's all we need as far as presence/absence info (correct me if I'm mistaken!).

Also, note that it's annoying [though still totally feasible] to pass pointers to POD and string literals in Go (*). Protobufs deals with this by providing helpers. We would probably do the same... See these guys:

http://godoc.org/code.google.com/p/goprotobuf/proto#String
http://godoc.org/code.google.com/p/goprotobuf/proto#Uint32

The above begs the question of whether there can/should be a central lib of Go thrift helpers that people can add to their projects (rather than simply embedding all of these, redundantly, in every generated thrift Go package).

So, let me know what you think. FYI, I know a lot about protobufs (was at google for 9 years), but less about Thrift per se. I'd love to know if there are thorny things I should be aware of in advance.

Also, if there's someone else who's willing to do this work, I have a million other things to do and would gladly wait for them if the turnaround would be less than a month or so. Thanks for your help!

> IsSet* broken in Go
> -------------------
>
>                 Key: THRIFT-2232
>                 URL: https://issues.apache.org/jira/browse/THRIFT-2232
>             Project: Thrift
>          Issue Type: Bug
>          Components: Go - Compiler
>            Reporter: Ben Sigelman
>              Labels: isset
>             Fix For: 0.9.2
>
>
> The various generated IsSetXYZ() methods just check for magic values of their respective fields. This is obviously broken, and also diverges from the implementation in other languages.
> I am willing and able to fix this myself, but I don't want to start on any impl until we can decide on an approach.
> At this point, though, optional fields in Go are basically useless if one's application makes use of the magic "absence" value.



--
This message was sent by Atlassian JIRA
(v6.1#6144)