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 <[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: Thu, 05 Dec 2019 18:21:51 +0000 Gerrit-HasComments: No
