You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/01/05 10:59:20 UTC
[kudu-CR] [tools] Add table scan tool
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12167
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional
SQL syntax, the scan tool's simple query predicates on columns
support three types of predicates generally, including 'Comparison',
'InList' and 'WhetherNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented by a single character '[', '(', '=', ')' or ']'
* The 'InList' type means values are in certain list,
which can be represented by a single character '@'
* The 'WhetherNull' type means whether the value is a NULL or not,
which can be represented by a single character 'i'(is) or '!'(is not)
One predicate entry should be represented as <column name>:<predicate type>:<value(s)>,
e.g. 'col1:[:lower;col1:]:upper;col2:@:v1,v2,v3;col3:!:NULL'
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 395 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/1
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#10).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 571 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/10
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#9).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 571 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/9
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#18).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 696 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/18
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 18
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#16).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 695 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/16
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 16
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#11).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_table.cc
6 files changed, 634 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/11
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#20).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 697 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/20
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 20
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Reviewed-on: http://gerrit.cloudera.org:8080/12167
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 697 insertions(+), 32 deletions(-)
Approvals:
Kudu Jenkins: Verified
Will Berkeley: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 21
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#4).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional
SQL syntax, the scan tool's simple query predicates on columns
support three types of predicates generally, including 'Comparison',
'InList' and 'WhetherNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented by a single character '[', '(', '=', ')' or ']'
* The 'InList' type means values are in certain list,
which can be represented by a single character '@'
* The 'WhetherNull' type means whether the value is a NULL or not,
which can be represented by a single character 'i'(is) or 'n'(is not)
One predicate entry should be represented as <column name>:<predicate type>:<value(s)>,
e.g. 'col1:[:lower;col1:]:upper;col2:@:v1,v2,v3;col3:n:NULL'
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 400 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/4
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#13).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 698 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/13
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 4:
(5 comments)
I left a few high-level comments, but at the top level: what is the goal of this tool? Is it to serve as a barebones shell, like impala-shell or MySQL shell? Is it meant for benchmarking scan performance?
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/kudu-tool-test.cc@2316
PS4, Line 2316: TEST_F(ToolTest, TestScanTable)
This requires a lot more tests. Correctness tests should cover at least the following things:
1. Scanning with no predicates and no columns for just a row count (Done).
2. Scanning with each predicate and checking it gets exactly the correct rows (includes matching, excludes non matching).
3. Scanning with projections of one or more columns and making sure the right columns are returned.
4. Scanning with multiple predicates on one column.
5. Scanning with multiple predicates across different columns.
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@18
PS4, Line 18: #include <stdlib.h>
Prefer #include <cstdlib>.
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@76
PS4, Line 76: "Query predicates on columns. Unlike the traditional"
Here and elsewhere, add "\n" to include linebreaks in the help message.
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@79
PS4, Line 79: WhetherNull
I think we can call this IsNull. It's shorter.
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@81
PS4, Line 81: '[', '(', '=', ')' or ']'"
I find these really confusing. For example, in the sample from the help message:
col1:[:lower;col1:]:upper;col2:@:v1,v2,v3;col3:n:NULL
My brain matches up [] pairs and wants to group together what's between them, so I want to interpret '[:lower;col1:]' as something meaningful when it's split between two predicates. Could we use friendlier symbols for the comparison operators? Can we just use < for <, <= for <=, etc?
Also, a few other things we ought to try and address with the syntax (or maybe just with documentation and help messages):
1. How can a user query for, e.g. 'KEY <= 10 AND KEY > 0'? Use two predicates?
2. Could we use = for equality and in-list predicates? E.g. 'col0:=:5' and 'col0:=:5,6,7,8'?
3. Why do we need the ":NULL" at the end of 'col3:n:NULL'? It's easier to parse, for sure, but it conveys no meaning.
4. How would a user do an equality predicate on a string with a comma in it?
I think it might be a good idea to use a simple JSON syntax for the predicates. This gives us a defined way to handle all the different characters, at the cost of some verbosity.
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 21:30:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#3).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional
SQL syntax, the scan tool's simple query predicates on columns
support three types of predicates generally, including 'Comparison',
'InList' and 'WhetherNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented by a single character '[', '(', '=', ')' or ']'
* The 'InList' type means values are in certain list,
which can be represented by a single character '@'
* The 'WhetherNull' type means whether the value is a NULL or not,
which can be represented by a single character 'i'(is) or 'n'(is not)
One predicate entry should be represented as <column name>:<predicate type>:<value(s)>,
e.g. 'col1:[:lower;col1:]:upper;col2:@:v1,v2,v3;col3:n:NULL'
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 402 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/3
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#17).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 695 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/17
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 17
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#19).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 697 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/19
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 19
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h@314
PS10, Line 314: if (direct_data_.empty()) {
: return KuduRowResult(projection_, nullptr);
: }
> Is this change related to the tool? What's its purpose?
Yes, it's a bug fix, some new added unit test will be failed. While projection_ is an empty schema, the direct_data_ be empty too, then we try to construct a KuduRowResult object with direct_data_[offset], it will encounter an invalid address access by direct_data_[0]
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@18
PS4, Line 18: #include <algorithm
> Prefer #include <cstdlib>.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 25 Jan 2019 13:21:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#6).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 568 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/6
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 11:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@285
PS10, Line 285:
: void RunActionS
> Could you add 'stderr' and 'SCOPED_TRACE(stderr)' back in here? Like how it
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@394
PS10, Line 394: }
: const string projection = JoinStrings(col_names, ",");
:
: v
> For this sort of pattern, it's also possible to accumulate the values in a
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@400
PS10, Line 400: Substitute("table s
> I think we want to use RunActionStdoutLines here. The tool should print the
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@407
PS10, Line 407: for (const auto& column : columns) {
: // Check matched rows.
: kvs.push_back(Substitute("$0 $1=$2",
: column.first, column.second, column.second == "key" ? to_string(value) : ".*"));
: }
: string line_pattern(R"*(\()*");
: line_pattern += JoinStrings(kvs, ", ");
: line_pattern += (")");
: ASSERT_STRINGS_ANY_MATCH(lines, line_pattern);
: }
: // Check total count.
> It'd be possible to simplify this a bit with JoinStrings-- you could get ri
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@421
PS10, Line 421:
> Remove.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2361
PS10, Line 2361:
: // Create the src table and write some data to it.
> Right now you are not using the client.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2365
PS10, Line 2365: e
> nit: Use a value instead of reference.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2383
PS10, Line 2383: ableName, &table));
> C++ raw string literals would help make this more readable. See https://en.
thanks for introducing it to me!
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2404
PS10, Line 2404: ,$0]])*", mid),
> You can use the 'client' you created above to insert one more row that has
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2414
PS10, Line 2414: total_rows, total_rows);
: RunScanTableCheck(kTableName,
> Not used.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2418
PS10, Line 2418:
> nit: Value instead of reference.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2420
PS10, Line 2420: }
:
: TEST_F(ToolTest, TestScanTableProjection) {
: NO_FATALS(StartExternalMiniCluster());
: string master_addr = cluster_->master()->bound_rpc_addr().ToString();
:
: const string kTableName = "kudu.table.scan.projection";
:
: // Create the src table and write some da
> I think using TestWorkload's default schema is fine, since this schema is j
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2454
PS10, Line 2454: NO_FATALS(StartExternalMiniCluster());
: string master_addr = cluster_->master()->bound_rpc_a
> Unused.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2460
PS10, Line 2460: TestWorkload ww(cluster_.get());
: ww.set_table_name(kTableName);
: ww.set_num_replicas(1);
: ww.set_write_pattern(TestWorkload::INSERT_SEQUENTIAL_ROWS);
: ww.set_num_write_threads(1);
: ww.Setup();
: ww.Start();
: ASSERT_EVENTUALLY([&]() {
: ASSERT_GE(ww.rows_inserted(), 1000);
> Again, should be able to use TestWorkload's default schema for this.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2498
PS10, Line 2498:
> Don't need this once the test works- the asserts should log the useful bit.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@91
PS10, Line 91: Builder builder;
: ReplicaController::SetVisibility(&builder, ReplicaController::Visibility::ALL);
: client::sp::shared_ptr<KuduClient> client;
: RETURN_NOT_OK(builder
> Update this in the same way as I suggested updating the commit message.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@95
PS10, Line 95: .master_server_addrs(master_addresses)
: .Build(&client));
> Why don't we want to show the values by default?
The most use scenario is to compare row count and time cost for each tablets to find out performance problem for bad designed schema, data skew, or some other reasons, so if we show the values by default, the terminal will be full of rows which are not we really care about. The secondary use scenario is to check the value.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@186
PS10, Line 186:
> If you need state, put it in a class.
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@200
PS10, Line 200: RETURN_NOT
> Please leave this as client::sp::shared_ptr. It's intentional that the pref
Done
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@445
PS10, Line 445: "as a JSON array" })
> I think this is enough extra code that it should go in its own file. Maybe
Done
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 26 Jan 2019 17:06:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#2).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional
SQL syntax, the scan tool's simple query predicates on columns
support three types of predicates generally, including 'Comparison',
'InList' and 'WhetherNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented by a single character '[', '(', '=', ')' or ']'
* The 'InList' type means values are in certain list,
which can be represented by a single character '@'
* The 'WhetherNull' type means whether the value is a NULL or not,
which can be represented by a single character 'i'(is) or 'n'(is not)
One predicate entry should be represented as <column name>:<predicate type>:<value(s)>,
e.g. 'col1:[:lower;col1:]:upper;col2:@:v1,v2,v3;col3:n:NULL'
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 401 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/2
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#7).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 570 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/7
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 15:
(2 comments)
One small thing and it's good to go! Thanks. This is a useful tool.
Looking forward, I think we eventually might want to specialize this tool into two different tools: a `kudu perf table_scan` tool that is meant to measure performance, and a `kudu table scan` tool that is meant to be used to see a few rows from a table or find specific rows. You plan to use the tool for the former, whereas I expect to use it more for the latter.
Right now both use cases can be done with the right combination of flags. In the future, it might be nice to wrap the same code with different defaults to cover the two use cases. Not needed for this patch though.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h@314
PS10, Line 314: if (direct_data_.empty()) {
: return KuduRowResult(projection_, nullptr);
: }
> Yes, it's a bug fix, some new added unit test will be failed. While project
Gotcha.
http://gerrit.cloudera.org:8080/#/c/12167/15/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/15/src/kudu/tools/tool_action_table.cc@469
PS15, Line 469: Scan rows from an exist table, several predicates can specified
Howabout: "Scan rows from an existing table. See the help for the --predicates flag on how predicates can be specified."
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 15
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 28 Jan 2019 19:52:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 4:
> (5 comments)
>
> I left a few high-level comments, but at the top level: what is the
> goal of this tool? Is it to serve as a barebones shell, like
> impala-shell or MySQL shell? Is it meant for benchmarking scan
> performance?
Yes, I think it's necessary to introduce a tool to benchmark the scan performance directly access the kudu server, but not the impala-shell or spark-shell, they may produce some extra time cost. And the scan tool build with in the kudu tool is some more convenient to use.
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 12 Jan 2019 09:32:51 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#5).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 567 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/5
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12167/15/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/15/src/kudu/tools/tool_action_table.cc@469
PS15, Line 469: Scan rows from an existing table. See the help "
> Howabout: "Scan rows from an existing table. See the help for the --predica
Done
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 17
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 29 Jan 2019 06:42:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 16:
Yes, and I saw some reduplicate code in tool_action_perf.cc, I will do some refactor after this patch has been merged.
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 16
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 29 Jan 2019 06:15:38 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#14).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 694 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/14
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 20: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 20
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 29 Jan 2019 17:38:56 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add table scan tool
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/4/src/kudu/tools/tool_action_table.cc@81
PS4, Line 81: '[', '(', '=', ')' or ']'"
> I find these really confusing. For example, in the sample from the help mes
yea, I was thinking the same thing -- rather than inventing a new syntax, I think a JSON representation of a parsed expression (AST-alike) would be much easier to reason about. Plus, we don't need to worry about implementing a parser :)
For example, ["and", ["<", "key", 123], ["notnull", "col2"]]. It sort of ends up being a lisp dialect, proving out Greenspun's tenth rule
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 22:22:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#12).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table. Several
predicates can specified on the query. Unlike traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
A src/kudu/tools/table_scanner.cc
A src/kudu/tools/table_scanner.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
9 files changed, 692 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/12
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tools] Add table scan tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12167 )
Change subject: [tools] Add table scan tool
......................................................................
Patch Set 10:
(25 comments)
Thanks for changing the query language! I think it's nicer now. It looks pretty good. I just had one big structural ask which is to separate all the code for the scanning into a file separate from tool_action_table.cc.
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@9
PS10, Line 9: , several
New sentence: ". Several..."
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@10
PS10, Line 10: the
Remove.
http://gerrit.cloudera.org:8080/#/c/12167/10//COMMIT_MSG@23
PS10, Line 23: All sub-predicates combined together as
: '[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
: operator is supported now.
For clarity, I'd rephrase this as:
Predicates can be combined together with predicate operators using the syntax
[operator, predicate, predicate, ..., predicate].
For example,
["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]
The only supported predicate operator is `AND`.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/client/scanner-internal.h@314
PS10, Line 314: if (direct_data_.empty()) {
: return KuduRowResult(projection_, nullptr);
: }
Is this change related to the tool? What's its purpose?
It's a semantic change because it doesn't look like the second arg to the KuduRowResult constructor would ever be nullptr in the original code.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@285
PS10, Line 285: SCOPED_TRACE(*stdout_lines);
: ASSERT_OK(s);
Could you add 'stderr' and 'SCOPED_TRACE(stderr)' back in here? Like how it is in 'RunActionStderrLines'.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@394
PS10, Line 394: string projections;
: for (const auto& column : columns) {
: projections += (column.second + ",");
: }
For this sort of pattern, it's also possible to accumulate the values in a vector and then use JoinStrings from kudu/gutil/strings/join.h:
vector<string> acc;
for (const auto& column : columns) {
acc.push_back(column.second);
}
const string projection = JoinStrings(acc, ",");
This way is fine though...up to you if you want to change it.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@400
PS10, Line 400: RunActionStderrString
I think we want to use RunActionStdoutLines here. The tool should print the rows on stdout, and we should check each line matches the expected format for a row.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@407
PS10, Line 407: string line_pattern(" \\(");
: int i = 0;
: for (const auto& column : columns) {
: // Check matched rows.
: line_pattern += Substitute("$0 $1=$2",
: column.first, column.second, column.second == "key" ? to_string(value) : ".*");
: if (++i < columns.size()) {
: line_pattern += (", ");
: }
: }
: line_pattern += (")");
It'd be possible to simplify this a bit with JoinStrings-- you could get rid of 'i', for example.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@421
PS10, Line 421: cost
Remove.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2361
PS10, Line 2361: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
Right now you are not using the client.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2365
PS10, Line 2365: &
nit: Use a value instead of reference.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2383
PS10, Line 2383: "[\"AND\",[\"=\",\"key\",1]]"
C++ raw string literals would help make this more readable. See https://en.cppreference.com/w/cpp/language/string_literal #6. In this case,
"[\"AND\",[\"=\",\"key\",1]]"
could be
R"*(["AND",["=","key",1]])*"
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2404
PS10, Line 2404: // TODO how to match?
You can use the 'client' you created above to insert one more row that has one column null. The default schema of the TestWorkload is
inline Schema GetSimpleTestSchema() {
return Schema({ ColumnSchema("key", INT32),
ColumnSchema("int_val", INT32),
ColumnSchema("string_val", STRING, true) },
1);
}
so the 'string_val' column is nullable. Then you'll get back one row here and one more row for all the other tests. The NOTNULL predicate test should be changed to be on 'string_val'.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2414
PS10, Line 2414: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
Not used.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2418
PS10, Line 2418: &
nit: Value instead of reference.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2420
PS10, Line 2420: KuduSchemaBuilder schema_builder;
: schema_builder.AddColumn("key")
: ->Type(client::KuduColumnSchema::INT32)
: ->NotNull()
: ->PrimaryKey();
: schema_builder.AddColumn("string_value")
: ->Type(client::KuduColumnSchema::STRING);
: KuduSchema schema;
: ASSERT_OK(schema_builder.Build(&schema));
I think using TestWorkload's default schema is fine, since this schema is just a projection of that one!
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2454
PS10, Line 2454: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
Unused.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2460
PS10, Line 2460: KuduSchemaBuilder schema_builder;
: schema_builder.AddColumn("key")
: ->Type(client::KuduColumnSchema::INT32)
: ->NotNull()
: ->PrimaryKey();
: schema_builder.AddColumn("string_value")
: ->Type(client::KuduColumnSchema::STRING);
: KuduSchema schema;
: ASSERT_OK(schema_builder.Build(&schema));
Again, should be able to use TestWorkload's default schema for this.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/kudu-tool-test.cc@2498
PS10, Line 2498: LOG(INFO) << line;
Don't need this once the test works- the asserts should log the useful bit.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@91
PS10, Line 91: "All sub-predicates combined together as\n"
: "'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND' "
: "operator is supported now.\n"
: " e.g. '[\"AND\", [\">=\", \"col1\", \"value\"], [\"NOTNULL\", \"col2\"]]'");
Update this in the same way as I suggested updating the commit message.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@95
PS10, Line 95: DEFINE_bool(show_value, false,
: "Whether to show values of scanned rows.");
Why don't we want to show the values by default?
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@186
PS10, Line 186: AtomicInt<uint64_t> total_count(0);
If you need state, put it in a class.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@200
PS10, Line 200: shared_ptr
Please leave this as client::sp::shared_ptr. It's intentional that the prefix is left because client::sp::shared_ptr and shared_ptr are not quite the same thing and we want to make it clear when the client::sp version is being used.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@445
PS10, Line 445: PredicateType ParsePredicateType(const string& predicate_type) {
I think this is enough extra code that it should go in its own file. Maybe table_scan.{h, cc} or something.
http://gerrit.cloudera.org:8080/#/c/12167/10/src/kudu/tools/tool_action_table.cc@678
PS10, Line 678: Status ScanTable(const RunnerContext &context) {
Make a class called TableScanner or whatever and put all this stuff in it, and put the class in a separate file. There here you just construct the TableScan object and call a few methods on it.
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 24 Jan 2019 21:12:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add table scan tool
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12167
to look at the new patch set (#8).
Change subject: [tools] Add table scan tool
......................................................................
[tools] Add table scan tool
This commit adds a basic tool to scan rows from a table, several
predicates can specified on the query. Unlike the traditional SQL
syntax, the scan tool's simple query predicates are represented in a
simple JSON syntax. Three types of predicates are supported, including
'Comparison', 'InList' and 'IsNull'.
* The 'Comparison' type support <=, <, ==, > and >=,
which can be represented as '[operator, column_name, value]',
e.g. '[">=", "col1", "value"]'
* The 'InList' type can be represented as
'["IN", column_name, [value1, value2, ...]]'
e.g. '["IN", "col2", ["value1", "value2"]]'
* The 'IsNull' type determine whether the value is NULL or not,
which can be represented as '[operator, column_name]'
e.g. '["NULL", "col1"]', or '["NOTNULL", "col2"]'
All sub-predicates combined together as
'[operator, sub-predicate1, sub-predicate2, ...]', and only 'AND'
operator is supported now.
e.g. '["AND", [">=", "col1", "value"], ["NOTNULL", "col2"]]')
Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
---
M src/kudu/client/scanner-internal.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 571 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12167/8
--
To view, visit http://gerrit.cloudera.org:8080/12167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieac340b70a9eaf131f82a2b7d61336211d1d48f8
Gerrit-Change-Number: 12167
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>