----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68664/#review208866 -----------------------------------------------------------
Basically this looks good. All my issues are pretty picky. standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsResponse.java Lines 38 (patched) <https://reviews.apache.org/r/68664/#comment293108> Why is this new interface marked Stable? I know this is generated code, but where does ths come from? standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1664 (patched) <https://reviews.apache.org/r/68664/#comment293104> nit: is a *list* of dot separated strings standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1704 (patched) <https://reviews.apache.org/r/68664/#comment293105> Maybe add: must be present but is ignored standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 2389 (patched) <https://reviews.apache.org/r/68664/#comment293106> this is get_partitions with filter and projectspec and unimplemented filter spec? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java Lines 49 (patched) <https://reviews.apache.org/r/68664/#comment293157> add class comment standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java Lines 57 (patched) <https://reviews.apache.org/r/68664/#comment293152> Add class description standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java Lines 111 (patched) <https://reviews.apache.org/r/68664/#comment293153> is used *to* parse standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java Lines 459 (patched) <https://reviews.apache.org/r/68664/#comment293155> use hive coding convention, if () { standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 1078 (patched) <https://reviews.apache.org/r/68664/#comment293147> add method description standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 1101 (patched) <https://reviews.apache.org/r/68664/#comment293148> add method description standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 1198 (patched) <https://reviews.apache.org/r/68664/#comment293132> Is it clearer to combine this bit of code with the similar calculation on line 120? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 65 (patched) <https://reviews.apache.org/r/68664/#comment293109> Add a brief comment explaining what the test does standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 84 (patched) <https://reviews.apache.org/r/68664/#comment293110> This seems weird standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 132 (patched) <https://reviews.apache.org/r/68664/#comment293111> nit: ask intellij to reformat this standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 161 (patched) <https://reviews.apache.org/r/68664/#comment293112> nit: OK you can hate me but I think builder methods should line up like return new PartitionBuilder() .inTable(table) .setValues(vals) .addPartParam("key1", "S1") .addPartParam("key2", "S2") .addPartParam(EXCLUDE_KEY_PREFIX + "key1", "e1") .addPartParam(EXCLUDE_KEY_PREFIX + "key2", "e2") .setBucketCols(table.getSd().getBucketCols()) .setSortCols(table.getSd().getSortCols()) .setSerdeName(table.getSd().getSerdeInfo().getName()) .setSerdeLib(table.getSd().getSerdeInfo().getSerializationLib()) .setSerdeParams(table.getSd().getSerdeInfo().getParameters()) .build(conf); standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 207 (patched) <https://reviews.apache.org/r/68664/#comment293114> use assertEquals And do you want to use static import for assert statements? I think it looks neater. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 234 (patched) <https://reviews.apache.org/r/68664/#comment293115> assertEquals standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 245 (patched) <https://reviews.apache.org/r/68664/#comment293117> inline this - though actually it always seems to be zero in all callers? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 278 (patched) <https://reviews.apache.org/r/68664/#comment293119> Shoudl we have the fields here that are not supported in direct sqL? serdeType, serializerClass and deserializerClass standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 300 (patched) <https://reviews.apache.org/r/68664/#comment293120> I couldn't work out what "i" was so pls document standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 309 (patched) <https://reviews.apache.org/r/68664/#comment293121> is 3 here just the length of "sd." ? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 338 (patched) <https://reviews.apache.org/r/68664/#comment293122> assertEquals standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 400 (patched) <https://reviews.apache.org/r/68664/#comment293123> weird that you set an exclude prefix with a method called include? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 442 (patched) <https://reviews.apache.org/r/68664/#comment293124> assertion message should be that key is found in the list? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 520 (patched) <https://reviews.apache.org/r/68664/#comment293127> reformat? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 568 (patched) <https://reviews.apache.org/r/68664/#comment293129> maybe this should be an old fashioned for loop? standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java Lines 579 (patched) <https://reviews.apache.org/r/68664/#comment293130> spelling: returned standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java Lines 396 (patched) <https://reviews.apache.org/r/68664/#comment293133> objects *with* null standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java Lines 404 (patched) <https://reviews.apache.org/r/68664/#comment293134> spelling: differeent standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java Lines 545 (patched) <https://reviews.apache.org/r/68664/#comment293136> Should be consistent: either use DB_NAME or "db1" but no both in same test. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java Lines 564 (patched) <https://reviews.apache.org/r/68664/#comment293135> nit: I'm on line 560 and I still am not used to this assrtThat/is stuff. You should not change this as there is so much code but I have not found this a useful addition to the usual style. standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java Lines 577 (patched) <https://reviews.apache.org/r/68664/#comment293137> name should contain One not one ? - Andrew Sherman On Sept. 20, 2018, 9:46 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68664/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2018, 9:46 p.m.) > > > Review request for hive, Aihua Xu, Andrew Sherman, Peter Vary, Todd Lipcon, > and Vihang Karajgaonkar. > > > Bugs: HIVE-20306 > https://issues.apache.org/jira/browse/HIVE-20306 > > > Repository: hive-git > > > Description > ------- > > HIVE-20306: Implement projection spec for fetching only requested fields from > partitions > > > Diffs > ----- > > > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java > 0ad2a2469e0330e050fdb8983078b80617afbbf1 > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsFilterSpec.java > PRE-CREATION > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsProjectSpec.java > PRE-CREATION > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsRequest.java > PRE-CREATION > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/GetPartitionsResponse.java > PRE-CREATION > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/PartitionFilterMode.java > PRE-CREATION > > standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > ae0956870a7d01c24f5fdaa07094c3dc6604ab9a > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php > 4574c6a4925ae3df9dd1ee7b8786976ae6fc8397 > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php > 22deffe1d31a64f95c49d7f017dfeb2994233e71 > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > a595732f04af4304974186178377192227bb80fb > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > 38074ce79b8a06b3795d00431025240778abb569 > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ttypes.py > 38fac465d73c264f85fc512548ebe1919ee35c17 > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/hive_metastore_types.rb > 0192c6da314694c1253b49949bbe749902f49b4b > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/thrift_hive_metastore.rb > e6a72762bb7b0d36fdf6d20d02cb1da3337a98a0 > standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift > 85a5c601e03ecd2fb6ac5d30d789193e10bf38c2 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > ba82a9327cf18e8d55ebddcd774786d3d72f753a > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > df6d56b679a03f88fe0de048d7b2d7e47709996c > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 54e7eda0da796877f1331de137d534126375c6ba > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > 571c789eddfd2b1a27c65c48bdc6dccfafaaf676 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java > PRE-CREATION > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > d27224b23580b4662a85c874b657847ed068c9a3 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PartitionProjectionEvaluator.java > PRE-CREATION > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java > b61ee81533930c889f23d2551041055cbdd1a6b2 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/StatObjectConverter.java > 7a0b21b2580d8bb9b256dbc698f125ed15ccdcd3 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java > 0445cbf9095285bdcde72946f1b6dd9a9a3b9fff > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/model/MSerDeInfo.java > 68f07e2569b6531cf3e18919209aed1a17e88bf7 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/model/MStorageDescriptor.java > 4c6ce008f89469353bfee3175168a518534a42b1 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java > 10ff9dfbb6d8f61fa75f731f4cd0f006c98e0067 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java > c681a87a1c6b10a4f9494e49a42282cf90027ad7 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 0934aeb3a7d5413cacde500a5575e4f676306bd0 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > 70a17f51b9b5a9fb0b5640988318fd39a82b895d > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java > ce590d0f5518e4eac570d7ba4885813202dfc0b5 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestGetPartitionsUsingProjection.java > PRE-CREATION > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java > 4937d9d861b13070e7df4f92bf434c40eb1538aa > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestPartitionProjectionEvaluator.java > PRE-CREATION > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/TestMetaStoreServerUtils.java > 30de1c4cfa1cf019186b10583a06da0bf5491634 > > > Diff: https://reviews.apache.org/r/68664/diff/2/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
