----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51912/#review172789 -----------------------------------------------------------
Hi Chris, Thank you for yout contribution, thisis a great change! There are a few places I found that can -and probably will- cause NullPointerExceptions, please fix these, as they're currently causing some tests to fail as well. It might be out of scope for this review, but it would be great if you could add new tests for to the TestHiveImport test class which can cover an external hive table import end to end (it could even be a parameterized test, so all tests would run once for external and once for non-external :)). Otherwise the change looks pretty nice and clear, thanks! :) Thanks, Anna src/java/org/apache/sqoop/hive/TableDefWriter.java Lines 136 (patched) <https://reviews.apache.org/r/51912/#comment245834> This line has a potential NPE in it (currently some tests run with ant test fail with NPE on one of these lines). Replacing it with StringUtils.isBlank() would help with this. It might also make sense to extract the whole condition like isHiveExternalTableSet or something similar src/java/org/apache/sqoop/hive/TableDefWriter.java Lines 142 (patched) <https://reviews.apache.org/r/51912/#comment245835> This line has a potential NPE. Replacing it with StringUtils.isBlank() would help with this. It might also make sense to extract the whole condition like isHiveExternalTableSet or something similar. src/java/org/apache/sqoop/hive/TableDefWriter.java Lines 231 (patched) <https://reviews.apache.org/r/51912/#comment245836> This line has a potential NPE in it. Replacing it with StringUtils.isBlank() would help with this. It might also make sense to extract the whole condition like isHiveExternalTableSet or something similar src/java/org/apache/sqoop/tool/BaseSqoopTool.java Lines 1583 (patched) <https://reviews.apache.org/r/51912/#comment245833> This line has a potential NPE. Replacing it with StringUtils.isBlank() would help with this. It might also make sense to extract the whole condition to make it a bit more easy to understand, e.g. isNotHiveImportButExternalTableDirIsSet or something similar :). src/test/org/apache/sqoop/hive/TestTableDefWriter.java Lines 82 (patched) <https://reviews.apache.org/r/51912/#comment245842> Thanks for creating a new test class! Would you please also add test cases that cover the "default path", where we're not creating external tables, to ensure that is still working as expected? - Anna Szonyi On April 21, 2017, 1:45 p.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51912/ > ----------------------------------------------------------- > > (Updated April 21, 2017, 1:45 p.m.) > > > Review request for Sqoop, Kathleen Ting, Attila Szabo, and Liz Szilagyi. > > > Repository: sqoop-trunk > > > Description > ------- > > SQOOP-816 - Hive External table support. Added --as-external-table option and > --table-location option > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java 801942e > src/java/org/apache/sqoop/hive/TableDefWriter.java 32fcca3 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 46f405f > src/test/org/apache/sqoop/hive/TestTableDefWriter.java PRE-CREATION > src/test/org/apache/sqoop/tool/TestImportTool.java 7e11f54 > > > Diff: https://reviews.apache.org/r/51912/diff/5/ > > > Testing > ------- > > Unit tests. > Interactive IDE debug step throughs. > No testing on HiveServer as SQOOP-2787 may not yet be in trunk. > End to end testing on PostgresQL to HiveServer performed. > > > Thanks, > > Chris Teoh > >
