You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2019/12/04 18:14:38 UTC
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Hello Adar Dembo, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14831
to review the following change.
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
KUDU-1938 Add VARCAHR to tooling and helpers pt 7
VARCHAR support was missing from some tools and helpers, adding them
here.
Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
---
M src/kudu/common/key_util.cc
M src/kudu/common/row.h
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
4 files changed, 31 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/14831/1
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-1938 Add VARCHAR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14831
to look at the new patch set (#4).
Change subject: KUDU-1938 Add VARCHAR to tooling and helpers pt 7
......................................................................
KUDU-1938 Add VARCHAR to tooling and helpers pt 7
VARCHAR support was missing from some tools and helpers, adding them
here.
Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
---
M src/kudu/common/row.h
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 31 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/14831/4
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14831
to look at the new patch set (#3).
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
KUDU-1938 Add VARCAHR to tooling and helpers pt 7
VARCHAR support was missing from some tools and helpers, adding them
here.
Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
---
M src/kudu/common/row.h
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 31 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/14831/3
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1938 Add VARCHAR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCHAR to tooling and helpers pt 7
......................................................................
KUDU-1938 Add VARCHAR to tooling and helpers pt 7
VARCHAR support was missing from some tools and helpers, adding them
here.
Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Reviewed-on: http://gerrit.cloudera.org:8080/14831
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/common/row.h
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 31 insertions(+), 4 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
Patch Set 1:
> Patch Set 1:
>
> Could you add appropriate test coverage to exercise the new bits?
The change in key_util.cc wasn't necessary after all as it checks physical types so the STRING case is unnecessary too. Could still add test and remove the STRING case.
The methods in row.h are only convenient methods, most of them are not used at all, but it might make sense to still add them.
The tool actions should be tested, but unfortunately they don't have tests at all (they would've likely caught the missing VARCHAR handling), but adding the tests would be a scope creep I believe. How do you feel about filing a JIRA to test the tools?
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 22:33:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
Patch Set 1:
Could you add appropriate test coverage to exercise the new bits?
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 18:33:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
Patch Set 2:
> The change in key_util.cc wasn't necessary after all as it checks physical types so the STRING case is unnecessary too. Could still add test and remove the STRING case.
>
> The methods in row.h are only convenient methods, most of them are not used at all, but it might make sense to still add them.
>
> The tool actions should be tested, but unfortunately they don't have tests at all (they would've likely caught the missing VARCHAR handling), but adding the tests would be a scope creep I believe. How do you feel about filing a JIRA to test the tools?
I asked for test coverage because, ultimately, the way in which we add a new data type is by grepping in the source code for the last data type to be added (now VARCHAR), then doing a bunch of copy/paste to create the new one. Without corresponding test coverage, it's easy to miss a piece and not notice. If there's test coverage, you have to miss both some piece _and_ its corresponding test. In that sense, the test functions as a sort of "parity bit".
That said, I don't want to burden you with testing brand new functionality; I'd rather you only extend existing tests where they exist.
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 22:56:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14831/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14831/3//COMMIT_MSG@7
PS3, Line 7: VARCAHR
VARCHAR
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 06 Dec 2019 05:21:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add VARCHAR to tooling and helpers pt 7
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCHAR to tooling and helpers pt 7
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 07 Dec 2019 00:42:37 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
Patch Set 2:
> Patch Set 2:
>
> > The change in key_util.cc wasn't necessary after all as it checks physical types so the STRING case is unnecessary too. Could still add test and remove the STRING case.
> >
> > The methods in row.h are only convenient methods, most of them are not used at all, but it might make sense to still add them.
> >
> > The tool actions should be tested, but unfortunately they don't have tests at all (they would've likely caught the missing VARCHAR handling), but adding the tests would be a scope creep I believe. How do you feel about filing a JIRA to test the tools?
>
> I asked for test coverage because, ultimately, the way in which we add a new data type is by grepping in the source code for the last data type to be added (now VARCHAR), then doing a bunch of copy/paste to create the new one. Without corresponding test coverage, it's easy to miss a piece and not notice. If there's test coverage, you have to miss both some piece _and_ its corresponding test. In that sense, the test functions as a sort of "parity bit".
Yep, that's how I wrote VARCHAR too and I missed these changes because didn't see these in DECIMAL.
>
> That said, I don't want to burden you with testing brand new functionality; I'd rather you only extend existing tests where they exist.
Unfortunately there are no existing tests for this.
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 05 Dec 2019 18:21:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add VARCAHR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14831
to look at the new patch set (#2).
Change subject: KUDU-1938 Add VARCAHR to tooling and helpers pt 7
......................................................................
KUDU-1938 Add VARCAHR to tooling and helpers pt 7
VARCHAR support was missing from some tools and helpers, adding them
here.
Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
---
M src/kudu/common/row.h
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
3 files changed, 29 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/14831/2
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1938 Add VARCHAR to tooling and helpers pt 7
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14831 )
Change subject: KUDU-1938 Add VARCHAR to tooling and helpers pt 7
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14831/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14831/3//COMMIT_MSG@7
PS3, Line 7: VARCHAR
> VARCHAR
Done
--
To view, visit http://gerrit.cloudera.org:8080/14831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84257c71528dafdf9c01c0d3cdda44a626ed7f67
Gerrit-Change-Number: 14831
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 06 Dec 2019 22:35:56 +0000
Gerrit-HasComments: Yes