----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66369/#review200335 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Line 1955 (original), 1959 (patched) <https://reviews.apache.org/r/66369/#comment281040> accidental change? ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java Lines 719 (patched) <https://reviews.apache.org/r/66369/#comment281041> LOG.debug ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java Line 842 (original) <https://reviews.apache.org/r/66369/#comment281042> Should this instead be (queryPlan.hasAcidResourcesInQuery() || (queryPlan.isMVRebuild()) or similar ? ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java Lines 960 (patched) <https://reviews.apache.org/r/66369/#comment281044> LOG.debug() ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java Lines 966 (patched) <https://reviews.apache.org/r/66369/#comment281045> Unused function. ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java Lines 1083 (patched) <https://reviews.apache.org/r/66369/#comment281046> LOG.warn() Also add in message that rebuild will fail now. ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java Lines 256 (patched) <https://reviews.apache.org/r/66369/#comment281047> Is arg dbName.TblName? If so, it should instead be dbName, tblName ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java Lines 221 (patched) <https://reviews.apache.org/r/66369/#comment281048> Can you add a comment on why it never actually locks. ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1358 (patched) <https://reviews.apache.org/r/66369/#comment281049> should this be diff < 0 ? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1352-1353 (original), 1376-1377 (patched) <https://reviews.apache.org/r/66369/#comment281081> Comment doesn't match code. Should this be outdated = false? ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1460 (patched) <https://reviews.apache.org/r/66369/#comment281125> This will update all MV before they are considered for rewrite. Looks like we are doing extra work which could be avoided by delaying augmenting MV until it qualifies for rewrite. ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1483 (patched) <https://reviews.apache.org/r/66369/#comment281082> It will be good to expand on how rewriting uses this info. ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java Lines 1522 (patched) <https://reviews.apache.org/r/66369/#comment281083> Is this guaranteed to be an acid table. Else, this may throw NPE when row_id is not present. ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java Lines 196 (patched) <https://reviews.apache.org/r/66369/#comment281050> LOG.debug ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAggregateIncrementalRewritingRule.java Lines 57-60 (patched) <https://reviews.apache.org/r/66369/#comment281137> Is this rewrite always better? Imagine tabA and tabB to be in millions rows but their join generates cross product of billions of rows. In prev case we were doing union of large MV with join of two tables (with which filter may get very small) as compared to here where we do ROJ of MV with one source table. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/MaterializedViewRewritingRelVisitor.java Lines 75 (patched) <https://reviews.apache.org/r/66369/#comment281140> should this check inner join ? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/MaterializedViewRewritingRelVisitor.java Lines 79 (patched) <https://reviews.apache.org/r/66369/#comment281141> Doesnt it need to check agg functions which are supported : min, max, count, sum ? ql/src/java/org/apache/hadoop/hive/ql/parse/MaterializedViewRebuildSemanticAnalyzer.java Lines 100 (patched) <https://reviews.apache.org/r/66369/#comment281146> LOG.debug ql/src/test/results/clientpositive/materialized_view_create_rewrite_3.q.out Line 443 (original), 443 (patched) <https://reviews.apache.org/r/66369/#comment281051> Lets move and keep all tests only on llap local. Testing on MR is not useful ql/src/test/results/clientpositive/materialized_view_create_rewrite_4.q.out Line 73 (original), 73 (patched) <https://reviews.apache.org/r/66369/#comment281147> Migrate test to run only on minillaplocal standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java Lines 2018 (patched) <https://reviews.apache.org/r/66369/#comment281052> We should instead pass dbName and tblName seperately. standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java Lines 2026 (patched) <https://reviews.apache.org/r/66369/#comment281053> Dbname, tblName. standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MaterializationsRebuildLockCleanerTask.java Lines 57 (patched) <https://reviews.apache.org/r/66369/#comment281148> LOG.info standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java Line 804 (original), 806 (patched) <https://reviews.apache.org/r/66369/#comment281150> accidental change? standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Lines 703-706 (patched) <https://reviews.apache.org/r/66369/#comment281152> These objects are not always needed. If so, we can delay getting them until needed. standalone-metastore/src/main/thrift/hive_metastore.thrift Lines 1164-1165 (original), 1164-1166 (patched) <https://reviews.apache.org/r/66369/#comment281151> Shouldn't they be required? standalone-metastore/src/main/thrift/hive_metastore.thrift Lines 2095-2096 (patched) <https://reviews.apache.org/r/66369/#comment281080> Api should have dbname, tbl name. - Ashutosh Chauhan On March 29, 2018, 8:56 p.m., Jesús Camacho Rodríguez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66369/ > ----------------------------------------------------------- > > (Updated March 29, 2018, 8:56 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-18839 > https://issues.apache.org/jira/browse/HIVE-18839 > > > Repository: hive-git > > > Description > ------- > > HIVE-18839 > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > 697b1940a928000d23e414025d990ef50d20ea69 > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java > 01cadeaaab2c8dd97b081d59baa06f450338027f > ql/src/java/org/apache/hadoop/hive/ql/exec/MaterializedViewTask.java > de120afbbcd02dc23dff163c14fd7c8fba25db0c > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java > 89ca1ff5cf1135cda85090ad2d03088b51680151 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java > 0db2a2c3ed5fda2352f2f63acbce8c6b4b889b84 > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java > c8cafa2a68f897b1034e5bf61e872e044e01c22a > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java > d76c8b6f40e8ab8a62875f6801d40b6c23c0ae85 > > ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveMaterializedViewsRegistry.java > 53dc8ec1974dfc095bd5c7601c8d486b4712319e > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAggregateIncrementalRewritingRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveNoAggregateIncrementalRewritingRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/MaterializedViewRewritingRelVisitor.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java > 41de17fd4679009ef6a4fb5a6d976cbc794ce791 > > ql/src/java/org/apache/hadoop/hive/ql/parse/MaterializedViewRebuildSemanticAnalyzer.java > 75eb50c5797b312f66e81b5cec23849684e641fc > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 26f20f2e05478fdbd05e494bf1a843567f2dff2c > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDTFGetSplits.java > 4c86fb89376041b30bc5c90a4e84f2e685d8ff82 > ql/src/test/queries/clientpositive/materialized_view_create_rewrite_4.q > c7f050be8da71f13463c22df2ccdab21c0e40b6a > ql/src/test/queries/clientpositive/materialized_view_create_rewrite_5.q > PRE-CREATION > ql/src/test/results/clientpositive/materialized_view_create_rewrite_3.q.out > 65614566c9b738ea0354dc6d206a0ed7e635174e > ql/src/test/results/clientpositive/materialized_view_create_rewrite_4.q.out > 48c0ecb23f1657c99ef30ed6963a3af1737f0514 > ql/src/test/results/clientpositive/materialized_view_create_rewrite_5.q.out > PRE-CREATION > standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h > e10a655e762eaaa5b0c32b2d1cf43a80a42410bc > standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp > 0f49d93277165e572c9b607eeed9ab3a168c1c0b > > standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp > d7319e247524d82603bc27d1bb791cbe1c79d740 > standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h > 724f02229b54f0e9a7ec8ab0546a83650860d1f1 > standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp > 8e357f6422fecfecac5a558c04ec741f06b87a62 > > standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Materialization.java > fec35d50b7009439da7f7b40911e4816996fb4fc > > standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java > e824f4a145cd049eb712b372dbf7fea3b7c36d42 > > standalone-metastore/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php > d00d11be3e054b805cb5c09c0222da2aa3a013f7 > standalone-metastore/src/gen/thrift/gen-php/metastore/Types.php > 32c8cb7bc57eb09a67bb971bcfe49155f6790a78 > > standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote > 1b3ebcf6ef9775785008bd3b549516f8932dd64f > > standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py > cf36654b5127a445577197dbf365352081e64e96 > standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py > fd157b2781f0746234ab62548855f14b5fc4ed64 > standalone-metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb > aa93158fa621e48552c0d108b68a12d8ed3589e9 > standalone-metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb > 7a07b73cc709684e8c548ebbbe4882d562f069e5 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > 8a5de09ccf1ebc21a86f479a65bb6e78defc3c31 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > d4bdcf1a79b18e61eb5d427646019a6dbcc9b4c0 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > c69192b73195dcfae3f2567d419ef0b0cf913a1e > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MaterializationInvalidationInfo.java > 3d774071c2ff00e892540364a7ec63e49b8089df > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MaterializationsInvalidationCache.java > 1636d48d2c7dd21ea25dd3d670103ee3c618df38 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MaterializationsRebuildLockCleanerTask.java > PRE-CREATION > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/MaterializationsRebuildLockHandler.java > PRE-CREATION > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > b8976ed953d1c449ce7d78b3cd37b39218e42bc2 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java > e453e5a7d2ec75a962194ae95a866aafee6e1b47 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java > 38fa0e2daa17fddda139087d20bc43573af0ea9c > standalone-metastore/src/main/thrift/hive_metastore.thrift > 68d7b45d0dae8bc8509a672c39dd57b029433e3c > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMaterializationsCacheCleaner.java > 6d3f68c0f0723dcdf15ee80de91a6c5a18c202ef > > > Diff: https://reviews.apache.org/r/66369/diff/1/ > > > Testing > ------- > > > Thanks, > > Jesús Camacho Rodríguez > >
