----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16184/#review30516 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java <https://reviews.apache.org/r/16184/#comment58439> can you add a javadoc comment describing the what the return value is ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java <https://reviews.apache.org/r/16184/#comment58440> can you add a javadoc comment describing the what the return value is ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java <https://reviews.apache.org/r/16184/#comment58441> This variable name was used before as well. But can you rename this variable to something more descriptive like opNotEOF, while you are making these changes? ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java <https://reviews.apache.org/r/16184/#comment58442> please use braces for the if condition - "if (ret) {.." ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java <https://reviews.apache.org/r/16184/#comment58443> duplicate if block. ql/src/java/org/apache/hadoop/hive/ql/io/HiveContextAwareRecordReader.java <https://reviews.apache.org/r/16184/#comment58438> In your comment in previous reviewboard link, you say that ObjectPair can't be used because "I need deep copy of the key and value field through ReflectionUtils". But the use of ReflectionUtils is not happening from within KVPair. I haven't understood why ObjectPair can't be used instead. ql/src/java/org/apache/hadoop/hive/ql/io/HiveContextAwareRecordReader.java <https://reviews.apache.org/r/16184/#comment58445> can you add this to the comment (unfortunately ReflectionsUtils.copy is poorly/incorrectly documented, created HADOOP-10168) // copy value from footerBuf to key,value ql/src/java/org/apache/hadoop/hive/ql/io/HiveContextAwareRecordReader.java <https://reviews.apache.org/r/16184/#comment58446> Please don't use same variable name as member variable, it gets very confusing! :) maybe, call it inputEOF ? pls add comment - //read new value into the buffer ql/src/java/org/apache/hadoop/hive/ql/io/HiveContextAwareRecordReader.java <https://reviews.apache.org/r/16184/#comment58447> looks like this else statement is not needed. the copy has already been done. Comments applicable to the FetchOperator also ql/src/test/queries/clientnegative/file_with_header_footer_negative.q <https://reviews.apache.org/r/16184/#comment58444> can you also add a test case with a table with empty file ? - Thejas Nair On Dec. 11, 2013, 9:19 p.m., Shuaishuai Nie wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16184/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2013, 9:19 p.m.) > > > Review request for hive, Eric Hanson and Thejas Nair. > > > Bugs: hive-5795 > https://issues.apache.org/jira/browse/hive-5795 > > > Repository: hive-git > > > Description > ------- > > Hive should be able to skip header and footer rows when reading data file for > a table > (follow up with review https://reviews.apache.org/r/15663/diff/#index_header) > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java fa3e048 > conf/hive-default.xml.template c61a0bb > data/files/header_footer_table_1/0001.txt PRE-CREATION > data/files/header_footer_table_1/0002.txt PRE-CREATION > data/files/header_footer_table_1/0003.txt PRE-CREATION > data/files/header_footer_table_2/2012/01/01/0001.txt PRE-CREATION > data/files/header_footer_table_2/2012/01/02/0002.txt PRE-CREATION > data/files/header_footer_table_2/2012/01/03/0003.txt PRE-CREATION > itests/qtest/pom.xml c3cbb89 > ql/src/java/org/apache/hadoop/hive/ql/exec/FetchOperator.java d2b2526 > ql/src/java/org/apache/hadoop/hive/ql/io/HiveContextAwareRecordReader.java > dd5cb6b > ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 974a5d6 > > ql/src/test/org/apache/hadoop/hive/ql/io/TestHiveBinarySearchRecordReader.java > 85dd975 > ql/src/test/org/apache/hadoop/hive/ql/io/TestSymlinkTextInputFormat.java > 0686d9b > ql/src/test/queries/clientnegative/file_with_header_footer_negative.q > PRE-CREATION > ql/src/test/queries/clientpositive/file_with_header_footer.q PRE-CREATION > ql/src/test/results/clientnegative/file_with_header_footer_negative.q.out > PRE-CREATION > ql/src/test/results/clientpositive/file_with_header_footer.q.out > PRE-CREATION > serde/if/serde.thrift 2ceb572 > > serde/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/serde/serdeConstants.java > 22a6168 > > Diff: https://reviews.apache.org/r/16184/diff/ > > > Testing > ------- > > > Thanks, > > Shuaishuai Nie > >
