Add method + implementation for retrieving table metadata#56
Add method + implementation for retrieving table metadata#56tdikland wants to merge 2 commits intodelta-incubator:mainfrom
Conversation
|
@roeap I'm not an expert at protobuf. Could you check if these definitions make sense? If validated, I'd be happy to implement the remaining methods of the protocol using the same approach. |
roeap
left a comment
roeap
left a comment
There was a problem hiding this comment.
@tdikland - looking great. left a few comments, but noting major.
I'd be curious as to your opinion on using protobuf to generate our types? Is the convenience of a structured interface definition and prospect of in principle being able to also quickly build a gPRC based sharing service worth the additional effort?
| optional string name = 2; | ||
| optional string description = 3; | ||
| optional ParquetFormat format = 4; | ||
| string schemaString = 5; |
There was a problem hiding this comment.
to be consistent, we should use snake_case for the message fields. The generated serde code will handle conversion for us, as this is luckily part of the json spec for proto messages :).
| delta_protocol: Some(t::DeltaProtocol { | ||
| min_reader_version: protocol.min_reader_version, | ||
| min_writer_version: protocol.min_writer_version, | ||
| reader_features: protocol | ||
| .reader_features | ||
| .clone() | ||
| .unwrap_or_default(), | ||
| writer_features: protocol | ||
| .writer_features | ||
| .clone() | ||
| .unwrap_or_default(), | ||
| }), |
There was a problem hiding this comment.
Should we define From implementations for the Protocol and Metadata types we get from kernel in the core crate?.
|
|
||
| message GetTableMetadataRequest { | ||
| // The share name to query. It's case-insensitive. | ||
| string share = 1 [(buf.validate.field).string.min_len = 1]; |
There was a problem hiding this comment.
just FYI - the buf.validate... annotations are neither required nor used right now :). But since there are constraints on these fields, and I wanted to test the protovalidate framework, I added some and just kept them, since they also do no harm.
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
Changes are included in this PR
Are there any user-facing changes?