----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14221/#review26274 -----------------------------------------------------------
Few comments, mostly around unnecessary null checks, which I think are no longer required, now that column pruning will always be happening. Secondly, I think we should be representing column list as LinkedHashSet instead of List. ql/src/java/org/apache/hadoop/hive/ql/exec/SMBMapJoinOperator.java <https://reviews.apache.org/r/14221/#comment51336> Seems like this null check is now redundant. List can never be null. ql/src/java/org/apache/hadoop/hive/ql/exec/SMBMapJoinOperator.java <https://reviews.apache.org/r/14221/#comment51341> If above is true, this else can also be removed. ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java <https://reviews.apache.org/r/14221/#comment51339> Seems like this null check is now redundant. Can we remove this? ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java <https://reviews.apache.org/r/14221/#comment51340> If above is true, than this can also be removed. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51337> Seems like this method is called only from tests, no one actually uses it. I will suggest just to remove this method altogether to minimize. appendReadColumnIds() can readily be used in place of this and that is what Hive uses everywhere. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51338> This method is also only called either from previous method and from test. Once we remove previous one, I don't think we need to introduce this new method. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51355> ids should be of type LinkedHashSet<Integer> instead of list. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51343> Seems like this null check is no longer needed. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51344> I don't think old can be null at this point either. We should remove this null check. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51356> Simlarly here cols should be of type LinkedHashSet<String> serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51345> This null check is not required anymore. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51342> Seems like there is no way that ids could be null now. Lets remove this null check. If someone is indeed passing null, than we are just masking that bug, which should be fixed at caller site. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51358> This should return LinkedHashSet<Integer> instead. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51346> Caller should never pass null conf, Returning empty list is dangerous. Better to let it throw NPE on the caller than this. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51347> It doesn't seem like that this method is needed. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51348> It should be caller's responsibility to not pass in null here. We should not do this null check. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51349> Remove call to this new public method and just inline that logic in this method. We should keep our public methods to minimum. serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java <https://reviews.apache.org/r/14221/#comment51350> Again, no null check please : ) - Ashutosh Chauhan On Sept. 19, 2013, 5:48 p.m., Yin Huai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14221/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2013, 5:48 p.m.) > > > Review request for hive. > > > Bugs: HIVE-4113 > https://issues.apache.org/jira/browse/HIVE-4113 > > > Repository: hive-git > > > Description > ------- > > Modifies ColumnProjectionUtils such there are two flags. One for the column > ids and one indicating whether all columns should be read. Additionally the > patch updates all locations which uses the old method of empty string > indicating all columns should be read. > > The automatic formatter generated by ant eclipse-files is fairly aggressive > so there are some unrelated import/whitespace cleanup. > > This one is based on https://reviews.apache.org/r/11770/ and has been rebased > to the latest trunk. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9f37d0c > conf/hive-default.xml.template 545026d > > hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableInputFormat.java > 766056b > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatBaseInputFormat.java > 553446a > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/HCatRecordReader.java > 3ee6157 > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/InitializeInput.java > 1980ef5 > > hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/TestHCatPartitioned.java > 577e06d > > hcatalog/hcatalog-pig-adapter/src/test/java/org/apache/hive/hcatalog/pig/TestHCatLoader.java > d38bb8d > ql/src/java/org/apache/hadoop/hive/ql/Driver.java 31a52ba > ql/src/java/org/apache/hadoop/hive/ql/exec/SMBMapJoinOperator.java ab0494e > ql/src/java/org/apache/hadoop/hive/ql/exec/TableScanOperator.java a5a8943 > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java 0f29a0e > ql/src/java/org/apache/hadoop/hive/ql/io/BucketizedHiveInputFormat.java > 49145b7 > ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java cccdc1b > ql/src/java/org/apache/hadoop/hive/ql/io/RCFile.java a83f223 > ql/src/java/org/apache/hadoop/hive/ql/io/RCFileRecordReader.java 9521060 > ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcInputFormat.java 50c5093 > > ql/src/java/org/apache/hadoop/hive/ql/io/rcfile/merge/RCFileBlockMergeRecordReader.java > cbdc2db > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/ColumnPrunerProcFactory.java > ed14e82 > ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java b97d869 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/MetadataOnlyOptimizer.java > 0550bf6 > ql/src/test/org/apache/hadoop/hive/ql/io/PerformTestRCFileAndSeqFile.java > fb9fca1 > ql/src/test/org/apache/hadoop/hive/ql/io/TestRCFile.java dd1276d > ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestInputOutputFormat.java > 83c5c38 > serde/src/java/org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java > 0b3ef7b > serde/src/java/org/apache/hadoop/hive/serde2/columnar/ColumnarSerDe.java > 11f5f07 > serde/src/java/org/apache/hadoop/hive/serde2/columnar/ColumnarStruct.java > 1335446 > > serde/src/java/org/apache/hadoop/hive/serde2/columnar/ColumnarStructBase.java > e1270cc > > serde/src/java/org/apache/hadoop/hive/serde2/columnar/LazyBinaryColumnarSerDe.java > b717278 > > serde/src/java/org/apache/hadoop/hive/serde2/columnar/LazyBinaryColumnarStruct.java > 0317024 > serde/src/test/org/apache/hadoop/hive/serde2/TestColumnProjectionUtils.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/14221/diff/ > > > Testing > ------- > > > Thanks, > > Yin Huai > >
