Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
......................................................................
Patch Set 22:
> Patch Set 22:
>
> "we'll ensure that both missing values and unknown values return an error per
> your ToClientEncodingType and equivalent functions".
> I'm sorry, I don't understand this place. I think missing values is a normal
> behavior, not an error. If the user does not pass, use the default value.
> Only unknown values need to return an error.
> Here I can define the enum type as int32, such as
> message CreateTablePB {
> enum EncodingType {
> UNKNOWN_ENCODING = 0;
> AUTO_ENCODING = 1;
> }
> optional int32 type = 1;
> }
> then call Foo_IsValid(int value) on the enum after parsing it from JSON. But
> user can't pass it as character string. If keeping using enum, I think we
> can't use 'JsonStringToMessage' or 'ParseFromString' directly. Their enum
> results are different in different versions or systems. Maybe I should parse
> it by myself. Or do some extra work with enum types. We need to determine if
> it exists first, and then you can tell if it's the right value.
OK, I think I see what you mean. To summarize:
- Known integer value: mapped to user-specified enum value. GOOD
- Known string value: mapped to user-specified enum value. GOOD
- Missing integer value: mapped to default enum value. GOOD
- Missing string value: mapped to default enum value. GOOD
- Unknown integer value: mapped to default enum value. BAD
- Unknown string value: return an error. GOOD
All the cases work out the way we want except for "unknown integer value",
which ideally we'd treat just like "unknown string value" and return an error.
But protobuf doesn't allow us to do that. Is this an accurate summary?
For what it's worth, I think we can live with this limitation, especially since
the alternatives all seem pretty bad. It's not ideal, but it's good enough.
--
To view, visit http://gerrit.cloudera.org:8080/14306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1
Gerrit-Change-Number: 14306
Gerrit-PatchSet: 22
Gerrit-Owner: YangSong <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong <[email protected]>
Gerrit-Comment-Date: Mon, 21 Oct 2019 06:13:47 +0000
Gerrit-HasComments: No
