> On Oct. 28, 2014, 8:52 a.m., Qian Xu wrote: > > src/test/org/apache/sqoop/manager/oracle/util/OracleData.java, line 160 > > <https://reviews.apache.org/r/27274/diff/1/?file=735263#file735263line160> > > > > Although `keySql` has a tailing space, but I'd suggest add a leading > > space before `"using"`.
Added space > On Oct. 28, 2014, 8:52 a.m., Qian Xu wrote: > > src/java/org/apache/sqoop/manager/oracle/OraOopManagerFactory.java, line 126 > > <https://reviews.apache.org/r/27274/diff/1/?file=735257#file735257line126> > > > > For better code readibility, consider make code shorter. > > > > OraOopOracleDataChunkMethod method = > > OraOopUtilities.getOraOopOracleDataChunkMethod(sqoopOptions.getConf(); > > > > if (method == > > OraOopConstants.OraOopOracleDataChunkMethod.PARTITION) { Split this into two lines > On Oct. 28, 2014, 8:52 a.m., Qian Xu wrote: > > src/java/org/apache/sqoop/manager/oracle/OraOopManagerFactory.java, line 706 > > <https://reviews.apache.org/r/27274/diff/1/?file=735257#file735257line706> > > > > Why remove the log? In case of removal, you can remove the variable > > result, as no other place actually use it. I moved the log entry to the accept method - as we now accept index organized tables if the chunk method is PARTITION - so there's an additional if statement in the accept method. I didn't want to change what the function did as the name of the function suggests it only checks if the table is an index organized table - this would be confusing if it also checked SQOOP parameters and said the table wasn't index organized as there is a parameter set. > On Oct. 28, 2014, 8:52 a.m., Qian Xu wrote: > > src/test/org/apache/sqoop/manager/oracle/ImportTest.java, line 252 > > <https://reviews.apache.org/r/27274/diff/1/?file=735262#file735262line252> > > > > Can you import some data and validate the records you have imported? There is another test which imports data and checks it. Currently these tests just import and make sure it ran successfully. We could enhance this to also check the data - but I think we should do this as a seperate issue as it will affect all the tests. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27274/#review58780 ----------------------------------------------------------- On Oct. 29, 2014, 3:24 a.m., David Robson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27274/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, 3:24 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1632 > https://issues.apache.org/jira/browse/SQOOP-1632 > > > Repository: sqoop-trunk > > > Description > ------- > > If an index organized table is partitioned - we can use the new chunk by > partition functionality on it - so we should add support for this. > IOTs that are not partitioned will still not be supported by the direct > connector. > > > Diffs > ----- > > src/docs/user/connectors.txt a118249 > src/java/org/apache/sqoop/manager/oracle/OraOopManagerFactory.java 9d75666 > src/java/org/apache/sqoop/manager/oracle/OraOopOracleQueries.java 7fd18a1 > src/test/oraoop/create_users.sql ecaa409 > src/test/oraoop/pkg_tst_product_gen.pbk 0bc7df7 > src/test/oraoop/table_tst_product_part_iot.xml PRE-CREATION > src/test/org/apache/sqoop/manager/oracle/ImportTest.java d914e3f > src/test/org/apache/sqoop/manager/oracle/util/OracleData.java 871d317 > src/test/org/apache/sqoop/manager/oracle/util/OracleTableDefinition.java > 5a8c42c > > Diff: https://reviews.apache.org/r/27274/diff/ > > > Testing > ------- > > Manually tested partitioned IOT. > Added automated test to create a partitioned IOT and run an import from it. > Ensure existing table types still work. > > > Thanks, > > David Robson > >
