[kudu-CR] add a tool to create table

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

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 <sy...@yeah.net>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong <sy...@yeah.net>
Gerrit-Comment-Date: Tue, 01 Oct 2019 07:09:39 +0000
Gerrit-HasComments: Yes