----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68395/#review208006 -----------------------------------------------------------
Very useful feature, thanks for doing this! A bunch of nits below. standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1645 (patched) <https://reviews.apache.org/r/68395/#comment291721> Can you put top-level comment for this struct explaining that this is an API to control both partition filtering and partition content. standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1647 (patched) <https://reviews.apache.org/r/68395/#comment291722> I think it is better to specify that API guarantees that it will include fields in the list although it may set some other fields as well - the contract is that *these fields* must be set. standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1654 (patched) <https://reviews.apache.org/r/68395/#comment291723> s/complaint/compliant/ standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1656 (patched) <https://reviews.apache.org/r/68395/#comment291725> Can you add some statement at the top telling that the API allows for param filtering as well. standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1659 (patched) <https://reviews.apache.org/r/68395/#comment291724> Can you specify what happens if some match is both included and excluded? standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift Lines 1668 (patched) <https://reviews.apache.org/r/68395/#comment291727> Do fields 1-6 belong here or in GetPartitionRequest? Essentially - are these part of the filter spec or part of the request asking for partitions? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 2612 (patched) <https://reviews.apache.org/r/68395/#comment291740> This wouldn't be a very useful MPart - will something set correct dbname, etc later? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 3358 (patched) <https://reviews.apache.org/r/68395/#comment291738> It would be good to add a comment that this is JDO-only code. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 3406 (patched) <https://reviews.apache.org/r/68395/#comment291736> mparts can be a huge list of objects, so printing this in debug log may be too much - may be just log number of objects? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 4035 (patched) <https://reviews.apache.org/r/68395/#comment291734> What about filtering params? Does it still apply? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 4051 (patched) <https://reviews.apache.org/r/68395/#comment291739> Nit: comments have spce after // standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 4052 (patched) <https://reviews.apache.org/r/68395/#comment291737> in setRange() or setResults()? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Line 4393 (original), 4508 (patched) <https://reviews.apache.org/r/68395/#comment291741> Is this legit change? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java Line 547 (original), 553 (patched) <https://reviews.apache.org/r/68395/#comment291733> Please document this API. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java Lines 554 (patched) <https://reviews.apache.org/r/68395/#comment291730> I don't think that `allowJDO` and `allowSql` belong here - this is an API for the raw store and jdo is an implementation detail of specific RawStore. So this should be an implementation that deals with it (probaby based on config or hardcore values) but we shouldn't be exposing this in the interface. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java Lines 555 (patched) <https://reviews.apache.org/r/68395/#comment291731> What if we later extern projectionSpec or filters to include more things? Would we need to add new methods here? Can we just pass either original Thrift request structs or some some representation of these so that we can extend things later without introducing new API methods? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java Lines 556 (patched) <https://reviews.apache.org/r/68395/#comment291732> When would it return `NoSuchObjectException`? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 991 (patched) <https://reviews.apache.org/r/68395/#comment291742> This is a public method, so it would benefit from documentation standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 1179 (patched) <https://reviews.apache.org/r/68395/#comment291743> Neat, didn't know about Objects.hash standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java Lines 1209 (patched) <https://reviews.apache.org/r/68395/#comment291744> Hmm, this looks suspicious. Objects can have the same hash code and be non-equal - shouldn't you compare values? Looks like you can use `Objects.equals` for this purpose. - Alexander Kolbasov On Aug. 27, 2018, 10:33 p.m., Vihang Karajgaonkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68395/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2018, 10:33 p.m.) > > > Review request for hive, Alexander Kolbasov, Alan Gates, Peter Vary, and > Sergey Shelukhin. > > > 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 > 47f96f323ade4e15753d6fd98709b9a882de624d > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php > 0973f4f3c13554b23ba59b8a1aa1c5a37c094a9e > > standalone-metastore/metastore-common/src/gen/thrift/gen-php/metastore/Types.php > 5ed4f71b1dd947c7d4cbb5c290b393b8cd2ea31d > > 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 > 3c0d0a55b1dcca96dca77676873b68e52703715d > > standalone-metastore/metastore-common/src/gen/thrift/gen-py/hive_metastore/ttypes.py > 7fc1e43de03eac1cfe802439ba38f83988299169 > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/hive_metastore_types.rb > e0c6c02715dab4d9ad457ec710bcb3159206c6c6 > > standalone-metastore/metastore-common/src/gen/thrift/gen-rb/thrift_hive_metastore.rb > e54a7321e2e1e6069a3e598627bc6f6eaed93449 > standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift > feb44d5159f131fae932739923b1a41f5e72e74b > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 33b22a9fc3e60cb6e11bec63d397f1fa712a41db > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > a0ff79cc5ca39f8af8bac672393e82d365c9fd4a > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > ac10da2f3e623c1fe2d1cbb0849af00c4520297b > > 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 > 4093aa7a18e52e1edbad2a872efe4d9b7cfe5b21 > > 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 > > > Diff: https://reviews.apache.org/r/68395/diff/9/ > > > Testing > ------- > > > Thanks, > > Vihang Karajgaonkar > >
