Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@937 PS5, Line 937: ParseValueOfType(column.default_value(), > I find there is a problem here. I can't use ParseValueOfType derictly, the Yeah this is frustrating; I didn't realize you'd need to _escape_ the JSON embedded inside the string values. That's super user-unfriendly. How about this alternative: define a ValuePB oneof and populate it with every public Kudu data type. I suggested this in https://gerrit.cloudera.org/c/14273/5/src/kudu/tools/tool_action_table.cc#897 but was worried that it would be difficult because there isn't a 1-1 mapping between Kudu and PB types. However, this isn't actually a problem, because I think you can define a oneof like this: message ValuePB { oneof value { string string_v; int32 int8_v; int32 int16_v; int32 int32_v; int64 int64_v; ... } } Meaning, you could include several scalars in the oneof that all use the same type even though each has a different semantic meaning. Then the remaining work is: 1. Validating that int8/int16/int32 aren't overflowed by whatever is in the int32 PB scalar. 2. Coming up with a suitable representation for DECIMAL. Probably something inside a string? The JSON representation isn't great as users will need to know to set string_v vs. int8_v etc. But at least there's a clear mapping from JSON field to Kudu data type, and it's easier to use than escaped JSON. I think it's also better than a PB Any type (https://developers.google.com/protocol-buffers/docs/proto3#any) though you should explore that option too. http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@970 PS5, Line 970: for (const auto& value : bound.lower_bound().bound_values()) { : values += value; : values += ","; : } > I try to modify like this: Right, because JoinKeysIterator() expects to operate on iterators belonging to a map. You'll need to use JoinStringsIterator or perhaps JoinElementsIterator. -- 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: 5 Gerrit-Owner: YangSong <sy1...@yeah.net> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <sy1...@yeah.net> Gerrit-Comment-Date: Tue, 01 Oct 2019 07:09:39 +0000 Gerrit-HasComments: Yes
