You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Ashutosh Chauhan <ha...@apache.org> on 2018/04/03 23:34:33 UTC

Re: Review Request 66369: HIVE-18839

-----------------------------------------------------------
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
> 
>


Re: Review Request 66369: HIVE-18839

Posted by Ashutosh Chauhan <ha...@apache.org>.

> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 1460 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990388#file1990388line1471>
> >
> >     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.
> 
> Jesús Camacho Rodríguez wrote:
>     Rewriting algorithm needs those filters to produce correct rewriting, hence though we are indeed doing extra work, I do not think we can avoid it. This part is also useful when MVs are outdated (not only for rebuild), as they may still be used to produce correct rewritings.
>     
>     On the other hand, we could avoid this work by implementing the pre-filtering techniques that we were talking about at some point. The idea would be that only views relevant for a given query will be returned by the method, then they would be enriched.
>     
>     Another improvement is that for a rebuild operation, we only consider in the rewriting the MV that we are rebuilding? However, this is a kind of heuristic, since other MVs that are up-to-date may be useful to rewrite the rebuild query.

Yeah, this makes pre-filtering even more important. I thought we had prefiltering based on table and col names. If not, we should add that in follow-up.
Heuristic for rebuidling makes lot of sense. Since I dont expect overlapping MVs to be common. Heuristic will also improve compile time.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAggregateIncrementalRewritingRule.java
> > Lines 57-60 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990390#file1990390line57>
> >
> >     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.
> 
> Jesús Camacho Rodríguez wrote:
>     Comment may be misleading. In the last MERGE, _source_ refers to _source_ block, not to a single query:
>     _(SELECT a, b, SUM(x) AS s, COUNT(*) AS c --NEW DATA
>      FROM TAB_A
>      JOIN TAB_B ON (TAB_A.a = TAB_B.z)
>      WHERE TAB_A.ROW_ID > 5
>      GROUP BY a, b) source_
>     
>     In any case, I think the point about performance is relevant. I expect if we do a rebuild with not many new rows, incremental maintenance will get the edge. However, if there are many changes in source tables, union may be benefitial over ROJ? But in the latter case, we also need to take into account that the UNION operation will invalidate the LLAP cache of the MV, while the ROJ variant will not. I can add a flag to control the incremental maintenance piece, even if it is always true, in case we want to disable it to do some tests in the future? What do you think?

yeah.. lets add a flag with default to true.


- Ashutosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66369/#review200335
-----------------------------------------------------------


On April 5, 2018, 2:23 a.m., Jesús Camacho Rodríguez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66369/
> -----------------------------------------------------------
> 
> (Updated April 5, 2018, 2:23 a.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 5f07312651dc64fe23dcb21d185d6676f33f3bb0 
>   itests/src/test/resources/testconfiguration.properties d2e077b5096cbf82f7b8a98e8f66a5ef98e72bc9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java fb1efe01dcddb4fd77f05918e46d742922dd313d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MaterializedViewTask.java 50fc4e0c63a060781c93f0e65c5da32dd9116d70 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 5a95649f5bccbdd44cb4c4d12c65d39a5a733a56 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 490f3b8ffe7564d10d86293b402d3c0dad2f7ee7 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java c8cafa2a68f897b1034e5bf61e872e044e01c22a 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eed37a19376c398cacd9f8e86551591eed89c07c 
>   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 99e2c72d212cfaefe463b1fa32239d6b63aa9228 
>   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/llap/materialized_view_create_rewrite_3.q.out 1ef7b876d82136e3a7ad9021db8c293a3cc4808a 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_4.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_5.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_rebuild_dummy.q.out 1ef7b876d82136e3a7ad9021db8c293a3cc4808a 
>   ql/src/test/results/clientpositive/llap/materialized_view_rewrite_7.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite.q.out 18eb1d1daa483d4635db514beb96104b558ee1bd 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite_2.q.out 144f9d84208ab32528a20bc6151c9be58571291e 
>   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_multi_db.q.out 32b408f5e4d389f9bd143695f8dd570db234a876 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite_time_window.q.out bfbac3f3492fdd15b55475cd423cf262793cff84 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_1.q.out 75e828fd4296d5f9333356916ba785ca664bfcb6 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_2.q.out 6e8b2e3e6593b9e921517cb349755a7b668dc900 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_3.q.out e3bd233177a9058cb202e641175a5fbb4aa7ca89 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_4.q.out 7301571cbf06b874fa027aa8e7a9bc1192a5c948 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_5.q.out bab3cacc2ce420578732037fb2cefc3d375c4544 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_6.q.out 017d793283904ca83fc1074eb01ca9879805a61a 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_7.q.out 486a50d55a346b7c2dd1e4e30a70ac7663da1d5d 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_8.q.out 1ca06d3cc4207818f059650b0bc250433d41c783 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_9.q.out 3120e0db7a19017d9edecc541f5bf1a1d444e66d 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_ssb.q.out bc42df022734c76c18dc32c64509dd16addd1d3f 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_ssb_2.q.out d561208fd8345bb338c0020ce6449fffd6067507 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h b9e8e24b6356feea2abd0777a1bd41907b6a4afc 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp c24055af98878a9422099e9e786252680243fcd5 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp cfec64f96a005dc6a342d267563f384108bfed22 
>   standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h 086cfe9db6cd64aa6720f63925ee501192e6008f 
>   standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp 383cb9bc9c1cb06cea4ed4577bcfe1bd81f69c54 
>   standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Materialization.java 69607280bb6fc92596db90c01ea2508fc2fe4cee 
>   standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 0bca4ebafd827bc1dd59b05f926bbeab97efb87f 
>   standalone-metastore/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php 5e3dff1a12a03bb5b362ba684e199f5afab589e4 
>   standalone-metastore/src/gen/thrift/gen-php/metastore/Types.php d4fcc888ed0ef679fa70ccc30c7d38fa932d5d8a 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote d39690f31ca62181cb535d95cc90203cfaec33d2 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py f8ffeac6053a894fb039c0fad3663f52249406d6 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py 972db1f0a39c8178b604e4d30447ee687243b5b3 
>   standalone-metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb 94454a1499e8354ef121074d1038a2f074a14cf5 
>   standalone-metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb c1036755b4421bfbcd4c21a036cc8bad5a8e32d8 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 8539fea42fa2743381833ab3137579caeac64672 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java ebbf465244885f8648800b0f98ec3abad9194fe1 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java b2c40c25f25c8069be9da198cc8f8c2dce09c14d 
>   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 80cb1de75e44efebd23e300c991b6715ad15faff 
>   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 995137f9675cd34b7d7c6f79bb78e206a65d99f7 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java 2b815fc2e199914dbbefd884f78e97b23fd4c880 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java e72d3278928e43f479de38f424c36298bb53e4da 
>   standalone-metastore/src/main/thrift/hive_metastore.thrift 7450439885f2437359db591802e8c37aae488226 
>   standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java 7d372627a4a0eb96c739ba7ecf30e2ab5799efb5 
>   standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMaterializationsCacheCleaner.java 7a871e14585b0cecd2e101609390e0b9c3672c8e 
> 
> 
> Diff: https://reviews.apache.org/r/66369/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jesús Camacho Rodríguez
> 
>


Re: Review Request 66369: HIVE-18839

Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.

> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java
> > Line 842 (original)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990385#file1990385line850>
> >
> >     Should this instead be (queryPlan.hasAcidResourcesInQuery() || (queryPlan.isMVRebuild())
> >      or similar ?

This is not part of this patch, it was coming from patch used to mv the id list generation. I have removed this.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java
> > Lines 966 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990385#file1990385line972>
> >
> >     Unused function.

It is used in L1115.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java
> > Lines 1083 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990385#file1990385line1089>
> >
> >     LOG.warn()
> >     Also add in message that rebuild will fail now.

This may also happen when we succeed, I have just transform it into debug.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java
> > Lines 221 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990387#file1990387line221>
> >
> >     Can you add a comment on why it never actually locks.

// This is default implementation. Locking only works for incremental maintenance
// which only works for DB transactional manager, thus we cannot acquire a lock.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 1352-1353 (original), 1376-1377 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990388#file1990388line1384>
> >
> >     Comment doesn't match code. 
> >     Should this be outdated = false?

Comment was outdated indeed, I have removed it. There is a comment just above that explains the logic. _outdated = true_ when _diff == 0L or we are rebuilding the MV_, and _the MV has been invalidated_, because in those cases it cannot be outdated at all.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 1460 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990388#file1990388line1471>
> >
> >     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.

Rewriting algorithm needs those filters to produce correct rewriting, hence though we are indeed doing extra work, I do not think we can avoid it. This part is also useful when MVs are outdated (not only for rebuild), as they may still be used to produce correct rewritings.

On the other hand, we could avoid this work by implementing the pre-filtering techniques that we were talking about at some point. The idea would be that only views relevant for a given query will be returned by the method, then they would be enriched.

Another improvement is that for a rebuild operation, we only consider in the rewriting the MV that we are rebuilding? However, this is a kind of heuristic, since other MVs that are up-to-date may be useful to rewrite the rebuild query.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 1522 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990388#file1990388line1533>
> >
> >     Is this guaranteed to be an acid table. Else, this may throw NPE when row_id is not present.

This should be an ACID table. _augment_ should not be called otherwise. I have added additional check on the invalidation information so we do not call the method.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAggregateIncrementalRewritingRule.java
> > Lines 57-60 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990390#file1990390line57>
> >
> >     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.

Comment may be misleading. In the last MERGE, _source_ refers to _source_ block, not to a single query:
_(SELECT a, b, SUM(x) AS s, COUNT(*) AS c --NEW DATA
 FROM TAB_A
 JOIN TAB_B ON (TAB_A.a = TAB_B.z)
 WHERE TAB_A.ROW_ID > 5
 GROUP BY a, b) source_

In any case, I think the point about performance is relevant. I expect if we do a rebuild with not many new rows, incremental maintenance will get the edge. However, if there are many changes in source tables, union may be benefitial over ROJ? But in the latter case, we also need to take into account that the UNION operation will invalidate the LLAP cache of the MV, while the ROJ variant will not. I can add a flag to control the incremental maintenance piece, even if it is always true, in case we want to disable it to do some tests in the future? What do you think?


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/MaterializedViewRewritingRelVisitor.java
> > Lines 75 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990392#file1990392line75>
> >
> >     should this check inner join ?

Currently rewritings with outer join are not produced, hence check is not needed unless I am missing something?


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/MaterializedViewRewritingRelVisitor.java
> > Lines 79 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990392#file1990392line79>
> >
> >     Doesnt it need to check agg functions which are supported : min, max, count, sum ?

If aggregation function is not supported, we cannot generate the rewriting. Supported functions are identified in _HiveRelBuilder.getRollup_.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
> > Lines 703-706 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990424#file1990424line703>
> >
> >     These objects are not always needed. If so, we can delay getting them until needed.

_materializationsRebuildLockHandler_ is needed by the _try_ and _finally_ clauses, hence I would not move it (we would need to call method twice).
_materializationsInvalidationCache_ is only needed in the _try_ clause, I have moved it.


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > standalone-metastore/src/main/thrift/hive_metastore.thrift
> > Lines 1164-1165 (original), 1164-1166 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990426#file1990426line1164>
> >
> >     Shouldn't they be required?

These two fields may not be set when Materialization is created. Then, when it is retrieved, they are populated with invalidation time and whether there was a source table update/delete when we retrieve them from the invalidation cache.


- Jesús


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66369/#review200335
-----------------------------------------------------------


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
> 
>


Re: Review Request 66369: HIVE-18839

Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.

> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
> > Lines 1460 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990388#file1990388line1471>
> >
> >     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.
> 
> Jesús Camacho Rodríguez wrote:
>     Rewriting algorithm needs those filters to produce correct rewriting, hence though we are indeed doing extra work, I do not think we can avoid it. This part is also useful when MVs are outdated (not only for rebuild), as they may still be used to produce correct rewritings.
>     
>     On the other hand, we could avoid this work by implementing the pre-filtering techniques that we were talking about at some point. The idea would be that only views relevant for a given query will be returned by the method, then they would be enriched.
>     
>     Another improvement is that for a rebuild operation, we only consider in the rewriting the MV that we are rebuilding? However, this is a kind of heuristic, since other MVs that are up-to-date may be useful to rewrite the rebuild query.
> 
> Ashutosh Chauhan wrote:
>     Yeah, this makes pre-filtering even more important. I thought we had prefiltering based on table and col names. If not, we should add that in follow-up.
>     Heuristic for rebuidling makes lot of sense. Since I dont expect overlapping MVs to be common. Heuristic will also improve compile time.

Prefiltering based on tables in present in Calcite, but we need to add the materializations to the planner first. Here logic is a bit different, because we are enriching the materializations before adding them to the planner. In any case, for the rebuild, which is the most common case of incremental rewriting, adding the heuristic will give us a boost. I just did it and added comments explaining it.

FWIW, in the creation metadata we have information about the tables used by a materialization, so we could do a simple prefilter before loading the materializations in the planner (follow-up).


> On April 3, 2018, 11:34 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/views/HiveAggregateIncrementalRewritingRule.java
> > Lines 57-60 (patched)
> > <https://reviews.apache.org/r/66369/diff/1/?file=1990390#file1990390line57>
> >
> >     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.
> 
> Jesús Camacho Rodríguez wrote:
>     Comment may be misleading. In the last MERGE, _source_ refers to _source_ block, not to a single query:
>     _(SELECT a, b, SUM(x) AS s, COUNT(*) AS c --NEW DATA
>      FROM TAB_A
>      JOIN TAB_B ON (TAB_A.a = TAB_B.z)
>      WHERE TAB_A.ROW_ID > 5
>      GROUP BY a, b) source_
>     
>     In any case, I think the point about performance is relevant. I expect if we do a rebuild with not many new rows, incremental maintenance will get the edge. However, if there are many changes in source tables, union may be benefitial over ROJ? But in the latter case, we also need to take into account that the UNION operation will invalidate the LLAP cache of the MV, while the ROJ variant will not. I can add a flag to control the incremental maintenance piece, even if it is always true, in case we want to disable it to do some tests in the future? What do you think?
> 
> Ashutosh Chauhan wrote:
>     yeah.. lets add a flag with default to true.

Added hive.materializedview.rebuild.incremental.


- Jesús


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66369/#review200335
-----------------------------------------------------------


On April 5, 2018, 2:23 a.m., Jesús Camacho Rodríguez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66369/
> -----------------------------------------------------------
> 
> (Updated April 5, 2018, 2:23 a.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 5f07312651dc64fe23dcb21d185d6676f33f3bb0 
>   itests/src/test/resources/testconfiguration.properties d2e077b5096cbf82f7b8a98e8f66a5ef98e72bc9 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java fb1efe01dcddb4fd77f05918e46d742922dd313d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/MaterializedViewTask.java 50fc4e0c63a060781c93f0e65c5da32dd9116d70 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java 5a95649f5bccbdd44cb4c4d12c65d39a5a733a56 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManager.java 490f3b8ffe7564d10d86293b402d3c0dad2f7ee7 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveTxnManagerImpl.java c8cafa2a68f897b1034e5bf61e872e044e01c22a 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java eed37a19376c398cacd9f8e86551591eed89c07c 
>   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 99e2c72d212cfaefe463b1fa32239d6b63aa9228 
>   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/llap/materialized_view_create_rewrite_3.q.out 1ef7b876d82136e3a7ad9021db8c293a3cc4808a 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_4.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_5.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_rebuild_dummy.q.out 1ef7b876d82136e3a7ad9021db8c293a3cc4808a 
>   ql/src/test/results/clientpositive/llap/materialized_view_rewrite_7.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite.q.out 18eb1d1daa483d4635db514beb96104b558ee1bd 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite_2.q.out 144f9d84208ab32528a20bc6151c9be58571291e 
>   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_multi_db.q.out 32b408f5e4d389f9bd143695f8dd570db234a876 
>   ql/src/test/results/clientpositive/materialized_view_create_rewrite_time_window.q.out bfbac3f3492fdd15b55475cd423cf262793cff84 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_1.q.out 75e828fd4296d5f9333356916ba785ca664bfcb6 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_2.q.out 6e8b2e3e6593b9e921517cb349755a7b668dc900 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_3.q.out e3bd233177a9058cb202e641175a5fbb4aa7ca89 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_4.q.out 7301571cbf06b874fa027aa8e7a9bc1192a5c948 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_5.q.out bab3cacc2ce420578732037fb2cefc3d375c4544 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_6.q.out 017d793283904ca83fc1074eb01ca9879805a61a 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_7.q.out 486a50d55a346b7c2dd1e4e30a70ac7663da1d5d 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_8.q.out 1ca06d3cc4207818f059650b0bc250433d41c783 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_9.q.out 3120e0db7a19017d9edecc541f5bf1a1d444e66d 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_ssb.q.out bc42df022734c76c18dc32c64509dd16addd1d3f 
>   ql/src/test/results/clientpositive/materialized_view_rewrite_ssb_2.q.out d561208fd8345bb338c0020ce6449fffd6067507 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h b9e8e24b6356feea2abd0777a1bd41907b6a4afc 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp c24055af98878a9422099e9e786252680243fcd5 
>   standalone-metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp cfec64f96a005dc6a342d267563f384108bfed22 
>   standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.h 086cfe9db6cd64aa6720f63925ee501192e6008f 
>   standalone-metastore/src/gen/thrift/gen-cpp/hive_metastore_types.cpp 383cb9bc9c1cb06cea4ed4577bcfe1bd81f69c54 
>   standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Materialization.java 69607280bb6fc92596db90c01ea2508fc2fe4cee 
>   standalone-metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java 0bca4ebafd827bc1dd59b05f926bbeab97efb87f 
>   standalone-metastore/src/gen/thrift/gen-php/metastore/ThriftHiveMetastore.php 5e3dff1a12a03bb5b362ba684e199f5afab589e4 
>   standalone-metastore/src/gen/thrift/gen-php/metastore/Types.php d4fcc888ed0ef679fa70ccc30c7d38fa932d5d8a 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote d39690f31ca62181cb535d95cc90203cfaec33d2 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py f8ffeac6053a894fb039c0fad3663f52249406d6 
>   standalone-metastore/src/gen/thrift/gen-py/hive_metastore/ttypes.py 972db1f0a39c8178b604e4d30447ee687243b5b3 
>   standalone-metastore/src/gen/thrift/gen-rb/hive_metastore_types.rb 94454a1499e8354ef121074d1038a2f074a14cf5 
>   standalone-metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb c1036755b4421bfbcd4c21a036cc8bad5a8e32d8 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 8539fea42fa2743381833ab3137579caeac64672 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java ebbf465244885f8648800b0f98ec3abad9194fe1 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java b2c40c25f25c8069be9da198cc8f8c2dce09c14d 
>   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 80cb1de75e44efebd23e300c991b6715ad15faff 
>   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 995137f9675cd34b7d7c6f79bb78e206a65d99f7 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java 2b815fc2e199914dbbefd884f78e97b23fd4c880 
>   standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java e72d3278928e43f479de38f424c36298bb53e4da 
>   standalone-metastore/src/main/thrift/hive_metastore.thrift 7450439885f2437359db591802e8c37aae488226 
>   standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java 7d372627a4a0eb96c739ba7ecf30e2ab5799efb5 
>   standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMaterializationsCacheCleaner.java 7a871e14585b0cecd2e101609390e0b9c3672c8e 
> 
> 
> Diff: https://reviews.apache.org/r/66369/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jesús Camacho Rodríguez
> 
>