-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AVRO-2773] Added support for logical types in C #843
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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