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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 04 Dec 2019 22:56:41 +0000 Gerrit-HasComments: No
