[AVRO-2773] Added support for logical types in C #843
Conversation
spektom
changed the title
spektom
changed the title
|
Hey guys, how about getting this merged. |
|
i was looking at that again just this morning! thanks |
|
I'm not sure these CI build errors are related. |
|
@spektom can you rebase against latest master? |
|
@Fokko build errors seem unrelated still. |
|
@Fokko tried again merging from master, I hope this time there won't be build issues. |
|
Hi, what is preventing this from being merged ? It would be very nice to have logical type support in the C library. |
|
I've not noticed any real owner for this project. Does this project have an owner anymore? |
|
Does it need an owner to get it merged? Is the owner the coder ? or is this related to the jira issue ? |
No, I'm not the owner. The PR is waiting for a review from @thiru-mg |
|
It seems that the reviewer is unresponsive, could we assign a new one ? |
|
@gscteam Please vote on the issue: https://issues.apache.org/jira/plugins/servlet/mobile#issue/AVRO-2773 |
|
I am interested in this as well. |
Please vote on the issue: https://issues.apache.org/jira/plugins/servlet/mobile#issue/AVRO-2773 |
|
Thanks. I voted. Let's all get the votes going over here: https://issues.apache.org/jira/browse/AVRO-2773 |
|
Likewise, voted 👍 |
|
Any chance to get this merged ? |
|
Any progress about this? |
IMO, the best you can do is vote for this fix here: https://issues.apache.org/jira/plugins/servlet/mobile#issue/AVRO-2773 |
|
I will close/reopen the PR to trigger the CI checks. |
|
Since there is no active maintainer of the C SDK I'd propose that at least two users/contributors review, test and approve this PR! |
|
@mkmkme @SahilKang Would you be interested in reviewing/testing this PR ? Thank you! |
|
Hey @martin-g ! I can test it, but it'll take some time. |
| @@ -48,98 +48,86 @@ void init_schema(void) | |||
| } | |||
| } | |||
|
|
|||
| /* Create a value to match the person schema and save it */ | |||
| /* Create a datum to match the person schema and save it */ | |||
There was a problem hiding this comment.
this switches the example code from the "value api" to the legacy "datum api"
| avro_datum_t avro_bytes(avro_schema_t schema, | ||
| const char *bytes, int64_t size) |
There was a problem hiding this comment.
this breaks backwards compatibility with the legacy api: stragglers that haven't updated their codebase to the current api probably won't be eager to do so for the sake of logical schema support
| struct avro_bytes_schema_t { | ||
| struct avro_obj_t obj; | ||
| avro_logical_schema_t *logical_type; | ||
| }; |
There was a problem hiding this comment.
I think to provide "first class" logical schema support, this approach would need to be reversed: instead of primitive/complex schemas containing logical schemas, the logical schemas should wrap their underlying types in some sense
by first class support, I mean that avro_type_t doesn't have any logical schema branches...which means that avro_value_iface::get_type alone wouldn't be enough to determine if a schema is logical
There was a problem hiding this comment.
Thanks for looking at the PR.
AFAIU, logical type is a non-mandatory annotation of a physical type, which is why [avro_type_t] is left without logical schema branches. For this reason, [avro_logical_schema_t] is implemented as a standalone decorator. Doesn't this make sense?
There was a problem hiding this comment.
I had started looking into ipc and logical support shortly before running into this PR, and managed to open #2891 as an alternative where I add decimal support by creating a new AVRO_DECIMAL avro_type_t branch
in that PR, the decimal support is added to the "value api" instead of the legacy "datum api", but in an optional way: users can call get_decimal on the new decimal values, or continue calling get_bytes or get_fixed
fwiw, the approach you've taken here is more similar to existing implementations from what I've seen: i.e. most implementations have a getType() and then a getLogicalType() method that users are expected to call if they're interested in logical types
in my opinion though (feel free to disagree), the distinction between logical and non-logical types is only useful for the spec and wire-format: from a user's pov, they're probably more interested in working with timestamps and decimals instead thinking about the underlying int and "big-endian encoded two's-complement" representations
AVRO-2773