Adar Dembo has posted comments on this change. Change subject: KUDU-1267: Calling getString() on RowResult for wrong type gave weird exception ......................................................................
Patch Set 1: (6 comments) When updating your patch, please don't abandon the gerrit and create a new one. It's unnecessary noise and makes it tougher to track the code review as a whole. You should be using 'git rebase' and/or 'git commit --amend' to modify your commit, then pushing it to gerrit _without_ modifying the Change-Id. Also, your editor appears to have made many indentation changes to RowResult.java. AFAICT they don't make the file any more or less compliant with a coding style, so could you please revert them? They're just unnecessary noise. http://gerrit.cloudera.org:8080/#/c/3102/1/java/kudu-client/src/main/java/org/kududb/client/RowResult.java File java/kudu-client/src/main/java/org/kududb/client/RowResult.java: Line 251: Nit: looks like you added two new empty lines here by mistake. Can you remove them? Line 494: throw new ClassCastException("Column (" + columnIndex + ") is of type " + columnType.getName() + " but was requested as a type " + expectedType.getName()); Nit: this line is too long; can you break it up? Also, why ClassCastException and not something more mundane like IllegalArgumentException or IllegalStateException? Line 568: } What happened here? http://gerrit.cloudera.org:8080/#/c/3102/1/java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java File java/kudu-client/src/test/java/org/kududb/client/TestKuduClient.java: Line 152: //Do negative testing on string type Nit: // Do negative... Line 154: if (scanner.hasMoreRows()) { If this is false, we won't actually run the remainder of the test. Shouldn't we assert that this is true? Line 157: boolean exceptionThrown = false; : try { : next.getInt("c2"); : } catch (ClassCastException e) { : exceptionThrown = true; : } : assertTrue("ClassCastException was not thrown when accessing a string column with getInt", exceptionThrown); : } Here's a simpler way to do it: try { next.getInt("c2"); fail("ClassCastException was not thrown..."; } catch (ClassCastException e) {} -- To view, visit http://gerrit.cloudera.org:8080/3102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idc8c6e5e382ca9f6280072f59acf4257fd13fc6b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ted Malaska <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
