[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 37: Thank you. :) -- 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: 37 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 25 Oct 2019 01:32:24 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Reviewed-on: http://gerrit.cloudera.org:8080/14306 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,849 insertions(+), 64 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 37 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 36: Code-Review+2 Just rebased since there was a conflict. -- 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: 36 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 24 Oct 2019 21:22:53 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Andrew Wong has uploaded a new patch set (#36) to the change originally created by YangSong. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,849 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/36 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 36 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 35: Code-Review+2 Thank you for making the change! :) -- 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: 35 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 24 Oct 2019 20:08:05 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#35). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,846 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/35 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 35 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#34). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,846 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/34 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 34 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#33). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,847 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/33 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 33 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#32). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,793 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/32 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 32 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 31: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc File src/kudu/tools/create-table-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc@1237 PS29, Line 1237: > OK. One last thing. Could you revert the structure of the test so that it lays out the schema and validation in order, similar to how you did in PS25? Doing it this current way makes it difficult to understand this test. -- 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: 31 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 24 Oct 2019 03:55:14 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#31). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,792 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/31 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 31 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 30: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc File src/kudu/tools/create-table-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc@1237 PS29, Line 1237: > This isn't quite what Adar meant by "fuzzing". Rather than selecting random OK. -- 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: 30 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 24 Oct 2019 03:11:44 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#30). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,794 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/30 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 30 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 29:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc
File src/kudu/tools/create-table-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14306/29/src/kudu/tools/create-table-tool-test.cc@1237
PS29, Line 1237: std::uniform_int_distribution distribution_bad(0,
bad_case_size - 1);
This isn't quite what Adar meant by "fuzzing". Rather than selecting randomly
from a set of pre-defined schemas, "fuzzing" means randomly generating a
schema, and then validating that if it's supposed to be correct (e.g. it was
generated as a correct schema), then the tool accepts it, and if it's not
supposed to be correct (e.g. in generating the schema, an error was injected,
like an incorrect enum), the tool rejects it.
Like Adar said, it may be a lot of work to do something like that here, and I
think I'd be happy merging this without fuzzing.
That said, since randomness here isn't really "fuzzing", I'd prefer removing
the new randomness and reverting back to running all these test cases (but we
can keep it in this create-table-tool-test)
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@981
PS25, Line 981:
> No, here is the EncodingType define:
I see, thanks for clarifying. Could you reword this then as:
"If no valid encoding is provided, AUTO_ENCODING will be used by default."
and below,
"If no valid compression is provided, DEFAULT_COMPRESSION will be used."
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@984
PS25, Line 984: if (column.has_compression()) {
> I find here has a warning during building. The warning message is "warning:
I see. I don't mind explicit declaration.
Although, take a look at consensus/leader_election.cc L164 and see if wrapping
these with GCC diagnostic ignored will help.
--
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: 29
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Thu, 24 Oct 2019 02:44:10 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#29). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,810 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/29 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 29 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 28:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5776
PS25, Line 5776:
> nit: What enum is this referring to? The bound_type? Maybe be more specific
Here include encoding type, compresssion type and range partition bound type.
All defined as enum.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6424
PS25, Line 6424:
> Isn't this also an error?
Yes, encoding type is defined as a enum type. Here using a invalid string,
error will be reported during parse the JSON to PB.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@981
PS25, Line 981: type);
> nit: which integer are you referring to? Do you mean if the column has no e
No, here is the EncodingType define:
enum EncodingType {
AUTO_ENCODING = 0;
PLAIN_ENCODING = 1;
PREFIX_ENCODING = 2;
RLE = 3;
DICT_ENCODING = 4;
BIT_SHUFFLE = 5;
}
The numbers 0 to 5 are correct, integer not in this range is
unknown integer value.
This place has a flaw, details are in the comments on PS 22.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@984
PS25, Line 984: // converted to DEFAULT_COMPRESSION.
I find here has a warning during building. The warning message is "warning:
‘type’ may be used uninitialized in this function". I initialize it at first,
but in patch set 16 as the advice of Adar I removed it. Does this place need to
remove this warning?
--
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: 28
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Wed, 23 Oct 2019 12:15:16 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#28). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,811 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/28 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 28 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#27). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/create-table-tool-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 7 files changed, 1,811 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/27 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 27 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#26). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,706 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/26 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 26 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 25:
(13 comments)
Nice work! I mostly have nit-picks.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/client/schema.cc@211
PS25, Line 211: encoding
Do we need to ToUpperCase() this here?
Ah I see where this is used that we do that elsewhere. Maybe rename the
"encoding" arg to "encoding_uc"? Or move the ToUpperCase() into this function?
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5390
PS25, Line 5390: TEST_F(ToolTest, TestCreateTable) {
nit: Maybe consider moving it into its own create_table-tool-test.cc that
includes tool_test_util.h, especially if you implement Adar's fuzzing
suggestion.
I worry that the larger file size makes kudu-tool-test even harder to index,
though I don't feel strongly about it.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5403
PS25, Line 5403: const string& master,
: const string& table_name,
: const string& schema,
: const string& partition,
: const map&
extra_configs,
: KuduClient* client) {
nit: add a few spaces to align with the (
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5412
PS25, Line 5412: Status s = RunKuduTool(table_args);
: ASSERT_TRUE(s.ok());
: bool table_exists = false;
: while (true) {
: s = client->TableExists(table_name, &table_exists);
: if (s.ok() || MonoTime::Now() > deadline) break;
: SleepFor(MonoDelta::FromMilliseconds(10));
: }
: ASSERT_TRUE(table_exists);
nit: maybe be more succinct as:
ASSERT_OK(RunKuduTool(table_args));
ASSERT_EVENTUALLY([&] {
ASSERT_OK(client->TableExists(table_name, &table_exists));
ASSERT_TRUE(table_exists);
});
This will retry the TableExists() call for up to 30 seconds with some backoff.
That way we also don't need 'deadline'.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5422
PS25, Line 5422: CHECK_OK
nit: ASSERT_OK so if it fails, it will fail more gracefully
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5776
PS25, Line 5776: enum_type_with_str
nit: What enum is this referring to? The bound_type? Maybe be more specific.
Also . instead of , and same elsewhere
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5838
PS25, Line 5838: invailed
"invalid", same elsewhere
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6104
PS25, Line 6104: : name:";
Does this print out "ID" too?
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6424
PS25, Line 6424: error_encoding
Isn't this also an error?
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6551
PS25, Line 6551: a existed table
nit: a table that already exists.
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@882
PS25, Line 882: KuduColumnStorageAttributes::AUTO_ENCODING;
: break;
: case ColumnPB::PLAIN_ENCODING :
: *type =
nit: there are some extra spaces here
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@931
PS25, Line 931: S
nit: there's an extra space here
http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@981
PS25, Line 981: Unknown integer value,
nit: which integer are you referring to? Do you mean if the column has no
encoding specified?
--
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: 25
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Wed, 23 Oct 2019 07:01:41 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 25: > Ok, I'll try to modify it as you said. It would be nice if you could provide > a similar example in the test code. Like I said, you've already done a ton of work so don't feel compelled to do that. But if you insist, here are some examples: - FsManagerTestBase_TestAddRemoveDataDirsFuzz (fs_manager-test.cc) - LogBlockManagerTest_TestMisalignedBlocksFuzz (log_block_manager-test.cc) - fuzz-itest.cc The common theme behind these tests is that it's too expensive to exhaustively test every possible combination of test input. So instead, the tests randomly generate one possible set of input and applies it. One test iteration doesn't tell us much, but the entire Kudu test suite runs over and over in precommit and in hourly/daily runs, so over time, we'll eventually cover most combinations and will know whether a particular combination fails. -- 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: 25 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 23 Oct 2019 04:03:20 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 25: Ok, I'll try to modify it as you said. It would be nice if you could provide a similar example in the test code. -- 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: 25 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 22 Oct 2019 05:41:33 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 25: Code-Review+1 Thanks for all of the hard work on this. It'd be nice if there was a "fuzz" test that randomly chose a bunch of options and passed those (via JSON) to create table, but you've already put a lot of work into this patch so I feel bad asking you to do that too. Anyway, given the complexity, it'd be great if someone else took a look at this. -- 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: 25 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 22 Oct 2019 04:34:31 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#25). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,745 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/25 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 25 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#24). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,745 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/24 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 24 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#23). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,746 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/23 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 23 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 22: Yes, that's a very accurate summary. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Mon, 21 Oct 2019 09:55:23 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Mon, 21 Oct 2019 06:13:47 +
Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
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.
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Mon, 21 Oct 2019 02:11:16 +
Gerrit-HasComments: No
[kudu-CR] add a tool to create table
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/22/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/22/src/kudu/tools/tool_action_table.cc@1095 PS22, Line 1095: // test code end > I add a test here. CreateTablePB is defined as: That's certainly unexpected. But it's workable: as long as we reserve 0 for the UNKNOWN value, we'll ensure that both missing values and unknown values return an error per your ToClientEncodingType and equivalent functions. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 18 Oct 2019 23:00:54 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14306/22/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/22/src/kudu/tools/tool_action_table.cc@1095
PS22, Line 1095: // test code end
I add a test here. CreateTablePB is defined as:
message CreateTablePB {
enum EncodingType {
UNKNOWN_ENCODING = 0;
AUTO_ENCODING = 1;
}
optional EncodingType type = 1;
}
run the test:
$ ./bin/kudu table create 1.1.1.1 {}
directly output:0
$ ./bin/kudu table create 1.1.1.1 {type:0}
type value: 0
directly output:0
$ ./bin/kudu table create 1.1.1.1 {type:1}
type value: 1
directly output:1
$ ./bin/kudu table create 1.1.1.1 {type:2}
directly output:0
At last case no error reported. So the empty object and the invalid value have
the same result. I have not found the reason, it's kind of like this problem:
https://groups.google.com/forum/#!msg/protobuf/wIeWEUFSfqg/CpcN3T4SAQAJ
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Fri, 18 Oct 2019 06:26:34 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#22). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,759 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/22 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 22 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 21:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc@937
PS21, Line 937: return Status::InvalidArgument(Substitute(
> I'm sorry, I made a mistake. If the integer value of enum is invalid, the o
Here are some experiments I ran with code I shared earlier:
$ ./test '{c : 1}'
Attempting to parse JSON: '{c : 1}'
c: RED
$ ./test '{c : 2}'
Attempting to parse JSON: '{c : 2}'
c: GREEN
$ ./test '{c : 3}'
Attempting to parse JSON: '{c : 3}'
unable to parse JSON: c: invalid value 3 for type TYPE_ENUM
This was with protoc 3.0.0; when I switched to protoc 3.4.0 (from Kudu's tree)
I got the "pass through" behavior you're talking about:
$ ./test '{c : 1}'
Attempting to parse JSON: '{c : 1}'
c: RED
$ ./test '{c : 2}'
Attempting to parse JSON: '{c : 2}'
c: GREEN
$ ./test '{c : 3}'
Attempting to parse JSON: '{c : 3}'
1: 3
Anyway, we need to handle this.
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum
has some useful advice: call bool Foo_IsValid(int value) on the enum after
parsing it from JSON. If we just parsed a string value, it'll always be valid.
But if we parsed an int value, we might now have an invalid enum that we should
reject.
--
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: 21
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Fri, 18 Oct 2019 04:38:35 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 21: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc@937 PS21, Line 937: return Status::InvalidArgument(Substitute( > From the PB guide, about enums: I'm sorry, I made a mistake. If the integer value of enum is invalid, the option field will be ignored during parsing the JSON object to PB. Not setting a default value. In the test, if the encoding type value is set to 200, anyway the table can be built successfully. If value in string format is invalid, error will be reported in parsing JSON object. -- 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: 21 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 18 Oct 2019 03:38:56 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 21: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc@937 PS21, Line 937: return Status::InvalidArgument(Substitute( > Yes, the enum can be specified as a string. If the value in string format i >From the PB guide, about enums: There must be a zero value, so that we can use 0 as a numeric default value. So maybe the right approach is to define UNKNOWN_ENCODING (and other unknown values in other enums) to 0? Also, what do you think of adding a default UNKNOWN_ value for BoundType? -- 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: 21 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 17 Oct 2019 18:57:02 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 21: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/21/src/kudu/tools/tool_action_table.cc@937 PS21, Line 937: return Status::InvalidArgument(Substitute( Yes, the enum can be specified as a string. If the value in string format is invailed, error reported in parsing JSON object. But if using a integer value for enum, the invailed value will be converted to 0, not the first value in enum, so if I define as '"encoding":100 ',it also can work. We can provide the user with a string format. This branch will not execute, maybe There is no need to define the 'UNKNOWN_ENCODING'. -- 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: 21 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 17 Oct 2019 08:46:18 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#21). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,654 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/21 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 21 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 20:
(5 comments)
I experimented with JSON serialization and enums, and I learned that you can
specify an enum as a integer value _or_ as a string. So we should definitely
use enums throughout, as this allows both the ease of use of specifying a
string value or the terseness of specifying an integer value, and the enum
definition itself provides a good source of documentation.
Here's my code:
test.proto:
syntax = "proto2";
message Foo {
enum Color {
BLUE = 0;
RED = 1;
GREEN = 2;
}
optional Color c = 1;
}
test.cc:
#include
#include
#include
#include
#include "test.pb.h"
using namespace std;
int main(int argc, char* argv[]) {
if (argc != 2) {
cout << "Usage: " << argv[0] << " " << endl;
return 1;
}
cout << "Attempting to parse JSON: \'" << argv[1] << "\'" << endl;
Foo message;
const auto& google_status =
google::protobuf::util::JsonStringToMessage(argv[1], &message);
if (!google_status.ok()) {
cout << "unable to parse JSON: " <<
google_status.error_message().ToString() << endl;
return 1;
}
cout << message.DebugString();
return 0;
}
I compiled it with:
- protoc --cpp_out . test.proto
- g++ -o test test.cc test.pb.cc -I. -lprotobuf
Then I ran it a few times:
$ ./test "{ c : 1 }"
Attempting to parse JSON: '{ c : 1 }'
c: RED
$ ./test "{ c : red }"
Attempting to parse JSON: '{ c : red }'
unable to parse JSON: Unexpected token.
{ c : red }
^
$ ./test "{ c : 'red' }"
Attempting to parse JSON: '{ c : 'red' }'
c: RED
$ ./test "{ c : 'green' }"
Attempting to parse JSON: '{ c : 'green' }'
c: GREEN
$ ./test "{ c : 'pink' }"
Attempting to parse JSON: '{ c : 'pink' }'
unable to parse JSON: c: invalid value "pink" for type TYPE_ENUM
$ ./test "{ c : 2 }"
Attempting to parse JSON: '{ c : 2 }'
c: GREEN
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool.proto@335
PS18, Line 335: // GROUP_VARINT encoding is deprecated and no longer
implemented.
: GROUP_VARINT = 3;
Should be removed in new code.
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool.proto@359
PS18, Line 359: Default type is AUTO_ENCODING, the types
: // PLAIN_ENCODING, PREFIX_ENCODING, GROUP_VARINT, RLE,
: // DICT_ENCODING, BIT_SHUFFLE are also supported.
Not necessary; it's obvious from the enum definition.
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool.proto@363
PS18, Line 363: Default type is DEFAULT_COMPRESSION, the
: // types NO_COMPRESSION, SNAPPY, LZ4, ZLIB are also supported.
Not necessary; it's obvious from the enum definition.
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool_action_table.cc@910
PS18, Line 910: column.encoding()));
> Here if the user uses an incorrect value, such as '"encoding" : 100', the v
Right, the way in which we usually handle that is by defining an
UNKNOWN_ENCODING (or UNKNOWN_COMPRESSION or whatever) enum entry that's first
in the PB enum. You could set it to a high value (like 999) to ensure that
users don't accidentally specify it, or just set it to 0. Either way, its
presence means that invalid values will be set to UNKNOWN_ENCODING, which you
can then filter for in the code and return an error if you see them.
You could do the same for INCLUSIVE/EXCLUSIVE if you want to catch mistakes
there too (vs. converting a mistake into INCLUSIVE).
Oh, I understand the problem now: you want these new enum values to match up
with the values defined in schema.h so you can pass them through 1-1. I'm not
sure if we should do that; it'll make it difficult to evolve one side without
also changing the other side.
http://gerrit.cloudera.org:8080/#/c/14306/20/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/20/src/kudu/tools/tool_action_table.cc@1017
PS20, Line 1017: unique_ptr
table_creator(client->NewTableCreator());
> 'table_creator' must be defined behind 'kudu_schema', otherwise in ASAN and
Makes sense; client.h says " /// Schema to use. Must remain valid for the
lifetime of the builder.".
--
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: 20
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 20: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/20/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/20/src/kudu/tools/tool_action_table.cc@1017 PS20, Line 1017: unique_ptr table_creator(client->NewTableCreator()); 'table_creator' must be defined behind 'kudu_schema', otherwise in ASAN and TSAN building, 'CreateTableTest' may fail. It has to do with the destructor order of these two variables. -- 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: 20 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 16 Oct 2019 09:40:48 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#20). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,502 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/20 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 20 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#19). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,502 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/19 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 19 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 18: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/18/src/kudu/tools/tool_action_table.cc@910 PS18, Line 910: column.encoding())); Here if the user uses an incorrect value, such as '"encoding" : 100', the value of column.encoding() will be converted to 0. And no errors are reported during converting JSON to PB. So I don't know if the user actually used the AUTO_ENCODING or if it was an error. -- 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: 18 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 16 Oct 2019 08:22:41 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#18). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,465 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/18 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 18 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 17: (3 comments) http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/client/schema.cc File src/kudu/client/schema.cc: PS17: You can remove the std:: prefixes in the new code. http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto@342 PS16, Line 342: // The encoding type of column. Default type is "AUTO_ENCODING", the types : // "PLAIN_ENCODING", "PREFIX_ENCODING", "GROUP_VARINT", "RLE", : // "DICT_ENCODING", "BIT_SHUFFLE" are also supported. : optional string encoding = 6; : // The compression of column. Default type is "DEFAULT_COMPRESSION", the : // types "NO_COMPRESSION", "SNAPPY", "LZ4", "ZLIB" are also supported. : optional string compression = 7; > If using PB enums, it maybe looks user-unfriendly. We talked about this in https://gerrit.cloudera.org/c/14306/3/src/kudu/tools/tool.proto#346. For now let's start with true enums where possible; if it turns out to be too confusing, we can change it later. http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/17/src/kudu/tools/tool_action_table.cc@1010 PS17, Line 1010: KuduSchema kudu_schema; Nit: move this definition to just before ParseTableSchema. The rule to follow here is to narrow the scope of a local variable as much as possible: this makes the code a little bit more readable (i.e. less state to track in your head). -- 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: 17 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 16 Oct 2019 03:43:08 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 17: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto@342 PS16, Line 342: // The encoding type of column. Default type is "AUTO_ENCODING", the types : // "PLAIN_ENCODING", "PREFIX_ENCODING", "GROUP_VARINT", "RLE", : // "DICT_ENCODING", "BIT_SHUFFLE" are also supported. : optional string encoding = 6; : // The compression of column. Default type is "DEFAULT_COMPRESSION", the : // types "NO_COMPRESSION", "SNAPPY", "LZ4", "ZLIB" are also supported. : optional string compression = 7; > Hmm, we should actually use PB enums for these, as we did for RangePartitio If using PB enums, it maybe looks user-unfriendly. -- 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: 17 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 16 Oct 2019 03:25:55 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#17). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,452 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/17 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 17 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 16:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/client/schema.h@170
PS16, Line 170: /// @param [in] String representation of the column encoding
type
: /// Column encoding type.
: /// @return Encoding type of the column.
: static Status StringToEncodingType(const std::string&
encoding,
: EncodingType* type);
:
: /// @param [in] String representation of the column
compression type
: /// Column compression type.
: /// @return Compression type of the column.
: static Status StringToCompressionType(const std::string&
compression,
: CompressionType* type);
Need to update the Doxygen comments.
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/client/schema.h@212
PS16, Line 212: /// @param [in] String representation of the column data type
: /// Column data type.
: /// @return Data type of the column.
Update this too.
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
PS16:
You can simplify (*type) to just *type
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/client/schema.cc@210
PS16, Line 210: Status s = Status::OK();
Status::OK() is the default value for Status; you don't need the assignment.
Below too.
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool.proto@342
PS16, Line 342: // The encoding type of column. Default type is
AUTO_ENCODING, the types
: // PLAIN_ENCODING, PREFIX_ENCODING, GROUP_VARINT, RLE,
DICT_ENCODING,
: // BIT_SHUFFLE are also supported.
: optional string encoding = 6;
: // The compression of column. Default type is
DEFAULT_COMPRESSION, the types
: // NO_COMPRESSION, SNAPPY, LZ4, ZLIB are also supported.
: optional string compression = 7;
Hmm, we should actually use PB enums for these, as we did for
RangePartitionPB.BoundPB.Type.
http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc@752
PS15, Line 752: *value = KuduValue::FromDecimal(precision, scale);
> Here I'm not sure if decimal type supports setting default value. And no si
Did you see what I wrote earlier?
https://gerrit.cloudera.org/c/14306/10/src/kudu/tools/tool_action_table.cc#753
http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc@1290
PS15, Line 1290: "\"lower_bound\":
{\"bound_type\":1,\"bound_values\": [\"2\"]},\""
> Here I just list an example, maybe too simple.
I think that's too verbose (and hard to format) to be useful help. Instead just
point people at tool.proto. We'll make sure the comments there are
understandable and clear.
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool_action_table.cc@806
PS16, Line 806: KuduColumnStorageAttributes::CompressionType compression_type
=
You don't need to initialize this; if we make it to L810, we're guaranteed to
have a value written into it.
Same below, and for encoding type.
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool_action_table.cc@888
PS16, Line 888: { return json_value; }, ",");
Ah, I was hoping L878-886 would be in the body of JoinMapped. Like this:
if (values.empty() || values.size() != range_columns.size()) {
return Status::InvalidArgument(Substitute(
"Invalid range value size, value size should be equal to number of
range keys."));
}
int i = 0;
string joined = JoinMapped(values, [&](const string& v) {
auto data_type = range_columns[i++].second;
if (data_type == KuduColumnSchema::STRING ||
data_type == KuduColumnSchema::BINARY) {
return "\"" + v + "\"";
}
return v;
}, ",");
*json_value = "[" + joined + "]";
return Status::OK();
http://gerrit.cloudera.org:8080/#/c/14306/16/src/kudu/tools/tool_action_table.cc@940
PS16, Line 940: vector primary_keys;
: for (const auto& primary_key : schema.key_column_names()) {
: primary_keys.push_back(primary_key);
: }
: b->SetPrimaryKey(primary_keys);
Does this work instead?
b->SetPrimaryKey(schem
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#16). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,481 insertions(+), 59 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/16 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 16 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 15: (2 comments) I modify the code according to the comments. Setting default value for DECIMAL type is not supported. http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc@752 PS15, Line 752: */ Here I'm not sure if decimal type supports setting default value. And no similar example has been found. I try to use `KuduValue::FromDecimal`, I split the value string with `.`, first value used as precision, second used as scale, but it doesn't look right. http://gerrit.cloudera.org:8080/#/c/14306/15/src/kudu/tools/tool_action_table.cc@1290 PS15, Line 1290: "\"3600\"}},\"num_replicas\":3}'.") Here I just list an example, maybe too simple. -- 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: 15 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 15 Oct 2019 10:06:51 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#15). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,476 insertions(+), 59 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/15 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 15 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#14). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,473 insertions(+), 59 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/14 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 14 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 13:
(23 comments)
Haven't reviewed the test code yet, but this is looking much better!
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
PS13:
Rather than introduce the UNKNOWN types, could we make these functions return a
Status (and pass the converted enum as an OUT parameter)? Then we could return
InvalidArgument if we don't recognize the string.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@338
PS13, Line 338: optional bool is_nullable = 3 [default = false];
That's the built-in default value for a bool.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@342
PS13, Line 342: // The following attributes refer to the on-disk storage of
the column.
: // They won't always be set, depending on context.
This comment is copied from common.proto but doesn't make sense here.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@347
PS13, Line 347: [default=0]
That's the built-in default value for an int32.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@413
PS13, Line 413: repeated ColumnPB schema = 2;
What do you think about defining a SchemaPB message, and putting both "repeated
ColumnPB columns" and "repeated string key_column_names" in it? It'll make it
easier to evolve the schema in the future, and it'll allow you to move the
primary key parsing into ParseKuduSchema more naturally.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@415
PS13, Line 415: repeated string primary_key = 3;
How about "key_column_names"? That explains that we're expecting more than one
entry, and that these are column names.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool.proto@421
PS13, Line 421: optional ExtraConfigPB extra_config = 6;
Nit: "extra_configs"
http://gerrit.cloudera.org:8080/#/c/14306/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/10/src/kudu/tools/tool_action_table.cc@753
PS10, Line 753: }
> Here I'm not sure if decimal type supports setting default value. Anad no s
KuduValue::FromDecimal should work, even for default values. See
decimal-itest.cc:79 for an example. Bear in mind the first arg to FromDecimal
is the raw int128_t value, not the precision.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@640
PS13, Line 640: vector>
range_name_type_map;
This isn't a map though. Maybe "range_col_names_and_types"?
Elsewhere too.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@870
PS13, Line 870: Status TransferValues(const RepeatedPtrField& values,
This is converting the PB values into a JSON representation of KuduPartialRow,
right? Maybe call it ToJsonPartialRow?
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@879
PS13, Line 879: for (int i = 0; i < values.size(); ++i) {
: const string& value = values[i];
: if (range_columns[i].second == KuduColumnSchema::STRING ||
: range_columns[i].second == KuduColumnSchema::BINARY) {
: (*v) += "\"";
: (*v) += value;
: (*v) += "\",";
: } else {
: (*v) += value;
: (*v) += ",";
: }
: }
Use JoinMapped from gutil/strings/join.h to simplify this.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@899
PS13, Line 899: (*b).
b->
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@908
PS13, Line 908: if (type != KuduColumnSchema::DECIMAL) {
: return Status::InvalidArgument(Substitute(
: "column $0 type attributes just can be used for
decimal type, not $1.",
: column.column_name(), column.column_type()));
: }
This is already validated in KuduSchemaBuilder::Build.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@967
PS13, Line 967: table_creator->add_hash_partitions(hash_keys,
hash_partition.num_buckets());
This is where you should use the optional seed.
http://gerrit.cloudera.org:8080/#/c/14306/13/src/kudu/tools/tool_action_table.cc@977
PS13, Line 977: vector(range_keys.begin(), range_keys.end()));
Nit: indent 4 chars further.
http://ge
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#13). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,406 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/13 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 13 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#12). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,407 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/12 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 12 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 11: > I can't understand the failure in the jenkins. In `IWYU.log` no > error message found. I suspect there is something wrong with the > code version. Right, it's not an error message, but IWYU says it wants you to fix include directives in src/kudu/tools/tool_action_table.cc Basically, you need to update your code as it says and submit the new version of the patch for review. You can save the following into the file (say, /tmp/iwyu.diff) and then apply it from the root of your kudu workspace: patch -p0 < /tmp/iwyu.diff # snip # --- src/kudu/tools/tool_action_table.cc +++ src/kudu/tools/tool_action_table.cc @@ -17,7 +17,6 @@ #include #include - #include #include #include @@ -32,6 +31,8 @@ #include #include #include +#include +#include #include #include # snip # -- 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: 11 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Sat, 12 Oct 2019 02:29:42 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 11: I can't understand the failure in the jenkins. In `IWYU.log` no error message found. I suspect there is something wrong with the code version. -- 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: 11 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 11 Oct 2019 11:45:00 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#11). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,406 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/11 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 11 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/14306/10/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/10/src/kudu/tools/tool_action_table.cc@753 PS10, Line 753: } Here I'm not sure if decimal type supports setting default value. Anad no similar example has been found. I try to use `KuduValue::FromDecimal`, I split the value string with `.`, first value used as precision, second used as scale, but it doesn't look right. -- 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: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 11 Oct 2019 09:45:45 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 10: Yes, the test case TestAlterColumn failed because the string value of compression type and encoding type are modified. I update the code. -- 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: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 11 Oct 2019 09:35:12 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#10). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,403 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/10 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#9). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,401 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/9 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 9 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#8). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 5 files changed, 1,402 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/8 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 8 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 7: Sorry haven't had time to rereview this yet, but looks like there are valid test failures, and there's some trailing whitespace and tabs to take care of. -- 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: 7 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Fri, 11 Oct 2019 05:13:15 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#7). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 6 files changed, 1,480 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/7 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 7 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc@665
PS6, Line 665: LOG(FATAL) << "Unhandled type " << type;
> I think this place needs to define a invalid data type in the enum, such as
Makes sense; that is a best practice we use elsewhere for protobuf enums.
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc@5433
PS6, Line 5433: "default_value": {"int_v": 1}
> Here I define a new PB:
The PB string type would work OK for most Kudu data types (ints, bool, float,
double, string), but how would it work for binary? Should we require base64
encoding?
You'd also need some special handling for decimal types, but I guess that would
have been the case with ValuePB too.
OK, I'm convinced: your suggestion is better, and we can make string work well
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: 6
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Tue, 08 Oct 2019 17:47:11 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 6:
(2 comments)
I try to use ValuePB to modify the code, the code now can work, but it looks
unfriendly to user. So not all the code is done, like support decimal type and
the test code. And some of the modified code is retained.
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/client/schema.cc@665
PS6, Line 665: LOG(FATAL) << "Unhandled type " << type;
I think this place needs to define a invalid data type in the enum, such as
"UNKNOWN_DATA = 999" in the common.proto. We need return the invalid data type
when user use a wrong parameter. The encode type and compression type have the
same problem.
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/14306/6/src/kudu/tools/kudu-tool-test.cc@5433
PS6, Line 5433: "default_value": {"int_v": 1}
Here I define a new PB:
message ValuePB {
oneof value {
int64 int_v = 1;
double double_v = 3;
bool bool_v = 4;
string string_v = 5;
}
}
but in the JSON, the value must be defined as `"default_value": {"int_v": 1}`.
That's also user-unfriendly. If using by a PB Any type, it also need be defined
as `"default_value": {1}`, it also doesn't look good. Maybe I need use the
string type instead of oneof type.
--
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: 6
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Tue, 08 Oct 2019 09:49:59 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#6). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 6 files changed, 1,387 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/6 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong 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(),
> Yeah this is frustrating; I didn't realize you'd need to _escape_ the JSON
I know what you mean, I will have a try.
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 += ",";
: }
> Right, because JoinKeysIterator() expects to operate on iterators belonging
Thank you.
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Tue, 08 Oct 2019 02:29:00 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Tue, 01 Oct 2019 07:09:39 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong 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)
The test code can define a new function to avoid repeating the same code over
and over.
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(),
> Wrap in RETURN_NOT_OK.
I find there is a problem here. I can't use ParseValueOfType derictly, there
are two problems:
1. the string should convert to list, like ["1"] or [1]
2. in the JSON object, the default value's type is always string, but if I
directly convert it to list, like "[1]" as the first parameter, it only work at
the type of int, if the type is string, we should convert "1" to "[\"1\"]".
Here I add a piece of code:
if (column.column_type() == "STRING" OR "BINARY") {
default_v = "[\"" + column.default_value() + "\"]";
} else {
default_v = "[" + column.default_value() + "]";
}
Do you think this is right? Or better advice. ConvertToKuduPartialRow has the
same problem.
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 += ",";
: }
> You should be able to use a function from gutil/strings/join.h here and bel
I try to modify like this:
string tmp = JoinKeysIterator(
bound.lower_bound().bound_values().begin(),
bound.lower_bound().bound_values().end(), ",");
but error reported at compile time. Maybe the RepeatedPtrField iterator is not
support.
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Sun, 29 Sep 2019 10:00:20 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
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:
(20 comments)
I haven't reviewed the new tests yet, but I glanced at them briefly and it
looks like there's a lot of potential to reuse code rather than repeating the
same code over and over for each create table.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/client/schema.cc@583
PS5, Line 583: const std::string& type) {
Indentation of the continuation still isn't right. Please review the Google
Style Guide link I dropped into a comment in an earlier revision.
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto@346
PS3, Line 346: message BoundPB {
> message RangeBoundPB {
I see what you mean: if we use an enum, users will have to specify a numeric
value instead of the friendlier "inclusive" or "exclusive" strings.
There's a trade-off though: if we go the string route, you'll need to write
more validation code to ensure the string values are one of two correct ones.
Furthermore, the schema itself won't really tell users what the legal values
are (without a comment anyway).
So I think enum is still the way to go. We can change it later if it proves to
be too confusing.
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto@378
PS3, Line 378:
> After removing "require" and "optional", there are errors reported at compi
Right, although we're using PB3, we're still using proto2 syntax (see L17),
which mandates required, optional, or repeated for each field.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool.proto@330
PS5, Line 330: message ColumnPB {
Need to include precision and scale for DECIMAL. Should probably include block
size too.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool.proto@341
PS5, Line 341: enum Type {
: EXCLUSIVE = 0;
: INCLUSIVE = 1;
: }
This definition can be scoped to BoundPB's definition.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool.proto@366
PS5, Line 366: message HashPartitionPB {
We should also support specifying a hash seed. See add_hash_partitions().
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool.proto@381
PS5, Line 381: message CreateTablePB {
We should support replication factor, extra configs, and dimension label. See
the KuduTableCreator class for details.
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@162
PS5, Line 162:
Global non-POD types are a bad idea in C++ as their initialization order is
undefined (leading to problems in library code).
Instead of this, how about declaring them statically as before, in new
functions that take a string as an argument and return either the EncodingType
or CompressionType?
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@517
PS5, Line 517:KuduColumnSchema::DataType>>&
range_colums,
This is a continuation of L516 so should be indented some more (aligned with
'string' ideally).
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@531
PS5, Line 531: if (values.size() != range_colums.size()) {
Should be called range_columns.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@754
PS5, Line 754: case KuduColumnSchema::DataType::DECIMAL:
We should probably address this limitation as it's perfectly legal for DECIMAL
columns to have default values. See KuduValue::FromDecimal for details.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@887
PS5, Line 887: "please use json_object or json_file flag to create a
table"));
Needs to be updated.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@931
PS5, Line 931: if (range_keys.find(column.column_name()) !=
range_keys.end()) {
Should use ContainsKey() from gutil/map-util.h.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@937
PS5, Line 937: ParseValueOfType(column.default_value(),
Wrap in RETURN_NOT_OK.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/tools/tool_action_table.cc@938
PS5, Line 938:
KuduColumnSchema::StringToDataType(column.column_type()), &value);
Indentation; see the GSG link. Below too.
http://gerrit.cloudera.org:8080/#/c/14306/5/src/kudu/too
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#4). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 6 files changed, 1,167 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/4 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 4 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#5). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc 6 files changed, 1,166 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/5 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 5 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto@346
PS3, Line 346: // lower bound value in format of string, include the
value.
message RangeBoundPB {
message BoundPB {
enum Type {
INCLUSIVE = 0;
EXCLUSIVE = 1;
}
optional Type bound_type = 1;
optional string bound_value = 2;
}
optional BoundPB lower_bound = 1;
optional BoundPB upper_bound = 2;
}
Thanks for helping. I modify here to support multi-column in range key. But is
using enum unfriendly here?
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool.proto@378
PS3, Line 378: optional
After removing "require" and "optional", there are errors reported at compile
time. So I change the "require" to "optional". I'm not sure if this is the
right change.
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool_action_table.cc@196
PS3, Line 196:
I define two variables from local to global. I'm not sure if it's right.
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool_action_table.cc@882
PS3, Line 882: if (json_arg[0] == '$') {
Here I misunderstood your suggestion. Maybe I just support the JSON object, the
JSON file can use 'cat' convert to JSON object. I will modify later.
http://gerrit.cloudera.org:8080/#/c/14306/3/src/kudu/tools/tool_action_table.cc@978
PS3, Line 978: // TODO: Mod ConvertToKuduPartialRow's parameter as
> warning: missing username/bug in TODO [google-readability-todo]
if using ConvertToKuduPartialRow, parameters need to be modified. I list an
example. Also first arg(const vector>& columns ) is OK.
--
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: 3
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Fri, 27 Sep 2019 07:55:17 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#3). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case.h 7 files changed, 1,142 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/3 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 3 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@341
PS1, Line 341: // lower bound value in format of string, include the
value.
: optional string in_lower_bound = 1;
: // lower bound value in format of string, exclude the
value.
: optional string ex_lower_bound = 2;
: // upper bound value in format of string, include the
value.
: optional string in_upper_bound = 3;
: // upper bound value in format of string, exclude the
value.
: optional string ex_upper_bound = 4;
> message RangeBoundPB {
Almost. Maybe this?
message RangeBoundPB {
message BoundPB {
enum Type {
INCLUSIVE = 0;
EXCLUSIVE = 1;
}
optional Type bound_type = 1;
optional string bound_value = 2;
}
optional BoundPB lower_bound = 1;
optional BoundPB upper_bound = 2;
}
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@911
PS1, Line 911: } else if (type == "DECIMAL") {
> Yes. but we should change the first parameter of function ConvertToKuduPart
Yeah, though you might want to combine the first two args (const
vector>& columns ) because both vectors are always
guaranteed to be the same length. Then you can figure out how to adapt a
KuduTable into that (by parsing its KuduSchema) for the existing call site.
--
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: 2
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Fri, 27 Sep 2019 05:22:32 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@330
PS1, Line 330: message ColumnPB {
> What about encoding and compression?
I add them.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@341
PS1, Line 341: // lower bound value in format of string, include the
value.
: optional string in_lower_bound = 1;
: // lower bound value in format of string, exclude the
value.
: optional string ex_lower_bound = 2;
: // upper bound value in format of string, include the
value.
: optional string in_upper_bound = 3;
: // upper bound value in format of string, exclude the
value.
: optional string ex_upper_bound = 4;
> Why not model this as two string values and two enums, like in add_range_pa
message RangeBoundPB {
message BoundPB {
// type of bound, "include" or "exclude"
optional string bound_type = 1;
// value of bound in type of string
optional string bound_value = 2;
}
repeate BoundPB bounds = 1;
}
define like this?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@350
PS1, Line 350: required string column = 1;
> Shouldn't this be repeated, to match set_range_partition_columns()?
Yes
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@351
PS1, Line 351: // range split bound.
> Do we also want to support splits (i.e. range partition key values that def
I will try to support.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@371
PS1, Line 371: required string table_name = 1;
> https://github.com/protocolbuffers/protobuf/issues/2497 explains why we don
I know, thank you.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
PS1:
> In general Kudu adheres to the Google C++ Style Guide. Please reformat cont
OK
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@877
PS1, Line 877: KuduValue* ParseValueOfType(const std::string& type,
> Can't we use the existing ParseValueOfType method, with some modifications
Yes.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@911
PS1, Line 911: } else if (type == "DECIMAL") {
> Why can't we reuse ConvertToKuduPartialRow?
Yes. but we should change the first parameter of function
ConvertToKuduPartialRow, like ConvertToKuduPartialRow(const vector&
columns, const vector& types; const string& range_bound_str,
KuduPartialRow* range_bound_partial_row) ?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1023
PS1, Line 1023: table_creator->set_range_partition_columns({});
> Use RETURN_NOT_OK instead of KUDU_CHECK_OK for cleaner failure behavior.
OK
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1031
PS1, Line 1031:
> Nit: KuduPartialRow*
Yes.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1263
PS1, Line 1263: .AddRequiredParameter({ kCompressionTypeArg, "Compression
type of the column" })
: .Build();
> This doesn't make sense to me; why not just a single required parameter tha
OK
--
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: 2
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Fri, 27 Sep 2019 03:24:30 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 ) Change subject: add a tool to create table .. Patch Set 2: I add some test code in patch set 2. I didn't have time to read the comments in PS1. I will reply the comments later. -- 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: 2 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 26 Sep 2019 12:22:26 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14306 to look at the new patch set (#2). Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case-test.cc M src/kudu/util/string_case.h 8 files changed, 1,267 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/2 -- 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: newpatchset Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 2 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table
..
Patch Set 1:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@330
PS1, Line 330: message ColumnPB {
What about encoding and compression?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@341
PS1, Line 341: // lower bound value in format of string, include the
value.
: optional string in_lower_bound = 1;
: // lower bound value in format of string, exclude the
value.
: optional string ex_lower_bound = 2;
: // upper bound value in format of string, include the
value.
: optional string in_upper_bound = 3;
: // upper bound value in format of string, exclude the
value.
: optional string ex_upper_bound = 4;
Why not model this as two string values and two enums, like in
add_range_partition()?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@350
PS1, Line 350: required string column = 1;
Shouldn't this be repeated, to match set_range_partition_columns()?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@351
PS1, Line 351: // range split bound.
Do we also want to support splits (i.e. range partition key values that define
a split point in range partitioning)? Modeled after add_range_partition_split().
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@368
PS1, Line 368: // create table protobuffer message. The JSON message provided
by user
Nit: got trailing whitespace here.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@371
PS1, Line 371: required string table_name = 1;
https://github.com/protocolbuffers/protobuf/issues/2497 explains why we don't
use required in new messages.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
PS1:
In general Kudu adheres to the Google C++ Style Guide. Please reformat
continuation lines for function calls as per the GSG guidance
(https://google.github.io/styleguide/cppguide.html#Function_Calls).
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@877
PS1, Line 877: KuduValue* ParseValueOfType(const std::string& type,
Can't we use the existing ParseValueOfType method, with some modifications for
DECIMAL?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@911
PS1, Line 911: Status SetColumnValue(const Slice& col_name, const std::string&
type,
Why can't we reuse ConvertToKuduPartialRow?
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1023
PS1, Line 1023: KUDU_CHECK_OK(b.Build(&kudu_schema));
Use RETURN_NOT_OK instead of KUDU_CHECK_OK for cleaner failure behavior.
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1031
PS1, Line 1031: KuduPartialRow *upper_bound = kudu_schema.NewRow();
Nit: KuduPartialRow*
http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1263
PS1, Line 1263: .AddOptionalParameter("json_file")
: .AddOptionalParameter("json_object")
This doesn't make sense to me; why not just a single required parameter that is
the JSON object?
Then the tool can be invoked in one of two ways:
1. kudu table create ""
2. kudu table create "$(cat /path/to/json/file)"
--
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: 1
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Sep 2019 06:22:02 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 10: https://gerrit.cloudera.org/#/c/14306/ the new code review -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Thu, 26 Sep 2019 03:04:05 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14306 Change subject: add a tool to create table .. add a tool to create table Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h A src/kudu/tools/testdata/create_hash_range_table_normal.txt M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case-test.cc M src/kudu/util/string_case.h 7 files changed, 418 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/14306/1 -- 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: newchange Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 1 Gerrit-Owner: YangSong
[kudu-CR] add a tool to create table
YangSong has abandoned this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Abandoned restart a code review. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 10: I'm sorry, maybe I have to create another code review. I think I have used the "git reset" command wrongly, now I can't commit and push my code now. I'm inexperienced with git. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Wed, 25 Sep 2019 11:49:41 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@342
PS7, Line 342: optional string in_lower_bound = 1;
> New PBs shouldn't use 'required'.
use optional? But the table name is indispensable.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@881
PS7, Line 881:type == "INT16" ||
:type == "INT32" ||
:type == "INT64" ||
:type == "UNIXTIME_MICROS") {
: int64_t v = 0;
:
> As we discussed, let's always retrieve the JSON blob as a CLI argument. If
Today there's something wrong with the Internet. Now I update my code. Please
help to review the code again. The tool can work now, but many test cases are
missing. I think I need to generate many test create table JSON files in
'src/kudu/tools/testdata/', and use them to test various scenarios. But I don't
know how to start a kudu service in the test. Can you give me some help?
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 10
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Wed, 25 Sep 2019 10:16:54 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#10). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h A src/kudu/tools/create_table_file M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case-test.cc M src/kudu/util/string_case.h 7 files changed, 418 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/10 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 10 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#9). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value.cc M src/kudu/common/partial_row.cc A src/kudu/tools/create_table_file M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case-test.cc M src/kudu/util/string_case.h 9 files changed, 420 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/9 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 9 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#8). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value.cc M src/kudu/common/partial_row.cc A src/kudu/tools/create_table_file M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case-test.cc M src/kudu/util/string_case.h 9 files changed, 406 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/8 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 8 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/client/value.h
File src/kudu/client/value.h:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/client/value.h@53
PS7, Line 53: static KuduValue* FromGeneral(const std::string& type, const
std::string& val);
This doesn't need to be part of KuduValue. It should be defined in
tool_action_table.cc, because it's only relevant to the kind of encoding used
by "kudu table create".
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/common/partial_row.h@112
PS7, Line 112: Status SetValue(const Slice& col_name, const std::string& type,
Likewise, this should also be in tool_action_table.cc because it shouldn't be
part of the general client API surface.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@329
PS7, Line 329:
Please document these new messages and their fields.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@342
PS7, Line 342: required string table_name = 1;
New PBs shouldn't use 'required'.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@343
PS7, Line 343: required SchemaPB schema = 2;
: optional PartitionSchemaPB partition = 3;
As discussed, we shouldn't expose internal PBs to CLI tooling, so that they can
evolve independently of the CLI. Please duplicate as needed, removing
irrelevant implementation details along the way.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@881
PS7, Line 881: const string& file_name = FindOrDie(context.required_args,
kCreateFileNameArg);
: int fd = open(file_name.c_str(), O_RDONLY);
: if (fd < 0) {
: return Status::InvalidArgument(Substitute(
: "create table json file open error"));
: }
As we discussed, let's always retrieve the JSON blob as a CLI argument. If a
user wants to pass it via file, they can do that via shell redirection.
On a related note, there is some precedence for JSON-based parsing of
values/rows. See the range partitioning CLI tools as well as column_set_default
(all in this file). Would any of that help here? If in the schema you specified
e.g. the range partition bound values as string, would the PB JSON conversion
for CreateTablePB leave those JSON snippets intact, allowing you to provide
custom handling using JsonReader?
Something like this:
1. Deserialize the entire JSON blob to a CreateTablePB.
2. Retrieve the strings for the e.g. default values for each column. Each one
is still in a JSON representation.
3. Use ParseValueOfType on each of those strings to convert them from JSON to a
KuduValue.
http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@887
PS7, Line 887: ControlShellProtocol
protocol(ControlShellProtocol::SerializationMode::JSON,
:
ControlShellProtocol::CloseMode::CLOSE_ON_DESTROY,
: fd, STDOUT_FILENO);
You shouldn't use this because you're not actually using the control shell.
Instead, duplicate whatever functionality out of ReceiveMessage that you need.
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 7
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Wed, 25 Sep 2019 04:41:04 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#7). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h A src/kudu/tools/create_table_file M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case.h 9 files changed, 284 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/7 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 7 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 6: > The PB message is from common.proto, the column type is enum-type, I don't > know if it fits. We shouldn't expose PB messages from common.proto via CLI tools; those are server-side and should be able to evolve independently of the tooling. Let's add new message definitions to tool.proto instead. > In the JSON object the column type defined as a integer, maybe not friendly. Yeah I think we should switch this to an enum to make it more user-friendly. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 24 Sep 2019 17:18:11 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 6: Thank you very much. I try to modify as discussed earlier in patch set 6. Current not support JSON object, I will add it later. The code maybe can't run. I want to make sure there is no problem with the implement solution first. And I will debug function and add test code later. The PB message is from common.proto, the column type is enum-type, I don't know if it fits. In the JSON object the column type defined as a integer, maybe not friendly. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 24 Sep 2019 11:44:38 + Gerrit-HasComments: No
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#6). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h A src/kudu/tools/create_table_file M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_table.cc M src/kudu/util/string_case.h 9 files changed, 284 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/6 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897 PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName", &table_name)); > I know what you mean, thank you. But I have a problem, JsonStringToMessage OK, I see what you're saying. The problem is that the PB type for a column's default value and for the values associated with range splits/partitions are dependent on other values in the message (i.e. the column types). One ugly work around is to define a generic KuduValue PB message which itself is a oneof on every possible PK type. That doesn't work super well when multiple Kudu types can be represented by a single PB type though (e.g. INT8/INT16/INT32 can all be represented by PB int32). As an alternative, maybe the value is always of type bytes or string (like you suggested) and then we'll need to do some follow-on validation and deserialization after deserializing the main JSON blob. I think even that is still better than the piecemeal approach taken here, because the PB schema is a useful artifact that describes the expected structure of the create table request. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 5 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Tue, 24 Sep 2019 00:42:13 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897
PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName",
&table_name));
> Yes, the idea is that rather than doing this piece-by-piece parsing with Js
I know what you mean, thank you. But I have a problem, JsonStringToMessage can
convert one type JSON into one type PB object, here column type is variable,
default value of column and range split value depend on it, maybe I can define
them to string type in the JSON, but the range split also has InBound and
ExBound two types. Now Range Split can be defined as:
"RangeSplit": [
{
"InLowerBound": "1",
"ExUpperBound": "2"
},
{
"InLowerBound": "2",
"InUpperBound": "3"
}
]
but if using PB, it maybe change as:
"RangeSplit": [
{
"LowerBound": {
"BoundType":"Include",
"BoundVaule":"1"
},
"UpperBound": {
"BoundType":"Exclude",
"BoundVaule":"2"
}
},
{
"LowerBound": {
"BoundType":"Include",
"BoundVaule":"2"
},
"UpperBound": {
"BoundType":"Include",
"BoundVaule":"3"
}
}
]
So I find the PB object can't be used directly and it become even more
complicated.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@1392
PS5, Line 1392: .AddRequiredParameter({ kCreateFileNameArg, "Name of the
table file" })
> I agree that it's easy to make a mistake when parsing the JSON, but I just
OK, I will try to support both, default reading from a JSON object.
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 5
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Mon, 23 Sep 2019 11:24:42 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897
PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName",
&table_name));
> I'm Sorry, I don't understand this place. The JSON schema is provided to th
Yes, the idea is that rather than doing this piece-by-piece parsing with
JsonReader, you define the user's create table schema as a protobuf object,
then use google::protobuf::util::JsonStringToMessage to convert it into the PB
object. Then you use the PB object in this function to extract the various bits
you need for KuduTableCreator.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@1392
PS5, Line 1392: .AddRequiredParameter({ kCreateFileNameArg, "Name of the
table file" })
> At first I want to define a JSON object, but the object is too long, it's v
I agree that it's easy to make a mistake when parsing the JSON, but I just
don't think a CLI tool should default to reading from a file like this. And if
you switch it to read the JSON as the last argument, it's really easy to use
shell redirection to get that argument from a file.
Put another way, reading from an argument makes the tool more composable: you
can either read the JSON object from the command line or a file.
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 5
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Mon, 23 Sep 2019 03:55:41 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 ) Change subject: add a tool to create table .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h File src/kudu/util/jsonreader.h: http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@95 PS5, Line 95: // Judge the field whether exist, return true if exist, if not return false > How about "Returns true if the field exists, false otherwise"? Yeah, My English is pool... http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@97 PS5, Line 97: const char* field); > Nit: fix indentation (should line up with 'const rapidjson...' from the lin OK -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 5 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong Gerrit-Comment-Date: Mon, 23 Sep 2019 03:20:35 + Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
YangSong has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@869
PS5, Line 869: Status CreateTable(const RunnerContext& context) {
> This function is enormous and could stand to be decomposed via helpers.
Yes, this function is too long. I will try to decompose it.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@870
PS5, Line 870: static unordered_map
data_type_map =
: {{"INT8", KuduColumnSchema::INT8},
: {"INT16", KuduColumnSchema::INT16},
: {"INT32", KuduColumnSchema::INT32},
: {"INT64", KuduColumnSchema::INT64},
: {"STRING", KuduColumnSchema::STRING},
: {"BOOL", KuduColumnSchema::BOOL},
: {"FLOAT", KuduColumnSchema::FLOAT},
: {"DOUBLE", KuduColumnSchema::DOUBLE},
: {"BINARY", KuduColumnSchema::BINARY},
: {"UNIXTIME_MICROS", KuduColumnSchema::UNIXTIME_MICROS},
: {"DECIMAL", KuduColumnSchema::DECIMAL}};
> We already have the opposite of this in KuduColumnSchema::DataTypeToString.
Yes, I modify it.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897
PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName",
&table_name));
> Looks like you're working against a predefined (even if implicit) JSON sche
I'm Sorry, I don't understand this place. The JSON schema is provided to the
user. Here parse it and use kudu::client::KuduTableCreator to create table.
Finally the JSON will transform to CreateTableRequestPB, then send to master.
Here I need provide a protobuf like CreateClusterRequestPB for user? using
protobuf instead of JSON?
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@1392
PS5, Line 1392: .AddRequiredParameter({ kCreateFileNameArg, "Name of the
table file" })
> I don't think we should force clients to use a file. Instead, let's just ta
At first I want to define a JSON object, but the object is too long, it's very
easy to write wrongly, and it's troublesome to revise, so I change to use a
json file as "src/kudu/tools/create_table_file". Of course it's easy to modify.
Or maybe both support?
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 5
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong
Gerrit-Comment-Date: Mon, 23 Sep 2019 03:18:32 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table
..
Patch Set 5:
(6 comments)
Could you add tests to kudu-tool-test to exercise the new functionality? Given
the various levers involved in table creation, maybe you could add a "fuzz"
test that randomly chooses a different (valid) configuration to exercise.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@869
PS5, Line 869: Status CreateTable(const RunnerContext& context) {
This function is enormous and could stand to be decomposed via helpers.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@870
PS5, Line 870: static unordered_map
data_type_map =
: {{"INT8", KuduColumnSchema::INT8},
: {"INT16", KuduColumnSchema::INT16},
: {"INT32", KuduColumnSchema::INT32},
: {"INT64", KuduColumnSchema::INT64},
: {"STRING", KuduColumnSchema::STRING},
: {"BOOL", KuduColumnSchema::BOOL},
: {"FLOAT", KuduColumnSchema::FLOAT},
: {"DOUBLE", KuduColumnSchema::DOUBLE},
: {"BINARY", KuduColumnSchema::BINARY},
: {"UNIXTIME_MICROS", KuduColumnSchema::UNIXTIME_MICROS},
: {"DECIMAL", KuduColumnSchema::DECIMAL}};
We already have the opposite of this in KuduColumnSchema::DataTypeToString.
Perhaps you could enhance that code to allow for type -> string and string ->
type lookups?
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@897
PS5, Line 897: RETURN_NOT_OK(reader.ExtractString(reader.root(), "TableName",
&table_name));
Looks like you're working against a predefined (even if implicit) JSON schema.
Could you define it as a protobuf, then use protobuf helper functions to
serialize/deserialize?
See tool.proto and tool_action_common.cc (ControlShellProtocol) for an example.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/tools/tool_action_table.cc@1392
PS5, Line 1392: .AddRequiredParameter({ kCreateFileNameArg, "Name of the
table file" })
I don't think we should force clients to use a file. Instead, let's just take a
JSON object as the argument; clients who wish to use a file can use shell
redirection to pipe the file contents into the call.
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h
File src/kudu/util/jsonreader.h:
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@95
PS5, Line 95: // Judge the field whether exist, return true if exist, if not
return false
How about "Returns true if the field exists, false otherwise"?
http://gerrit.cloudera.org:8080/#/c/14273/5/src/kudu/util/jsonreader.h@97
PS5, Line 97: const char* field);
Nit: fix indentation (should line up with 'const rapidjson...' from the line
before).
--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 5
Gerrit-Owner: YangSong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Sep 2019 23:38:37 +
Gerrit-HasComments: Yes
[kudu-CR] add a tool to create table
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14273 to look at the new patch set (#5). Change subject: add a tool to create table .. add a tool to create table Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 --- A src/kudu/tools/create_table_file M src/kudu/tools/tool_action_table.cc M src/kudu/util/jsonreader-test.cc M src/kudu/util/jsonreader.cc M src/kudu/util/jsonreader.h 5 files changed, 400 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/14273/5 -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 5 Gerrit-Owner: YangSong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
