You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jesús Camacho Rodríguez <jc...@hortonworks.com> on 2020/05/22 22:18:36 UTC
Re: Review Request 72433: Extract Create View analyzer from
SemanticAnalyzer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72433/#review220850
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java
Line 442 (original)
<https://reviews.apache.org/r/72433/#comment309568>
Why is this error code going away? Even if it is not used probably makes sense to keep it around and mark it as deprecated for backwards reference.
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java
Lines 49 (patched)
<https://reviews.apache.org/r/72433/#comment309566>
It seems more appropiate to update the display name too at this point.
ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java
Lines 67 (patched)
<https://reviews.apache.org/r/72433/#comment309567>
// only used for materialized views
These comments can go away.
ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java
Line 1818 (original), 1818 (patched)
<https://reviews.apache.org/r/72433/#comment309569>
Can we make this protected again? I did not see any usage that is not by a subclass?
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
Lines 566 (patched)
<https://reviews.apache.org/r/72433/#comment309570>
What is the goal of this block? Why is this needed now and it was not needed before? This seems more than a refactoring.
Fwiw there is some logic below executed when we create a view in _handleCreateViewDDL_.
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
Line 1848 (original), 1854 (patched)
<https://reviews.apache.org/r/72433/#comment309571>
Why are we changing this? We should not have to change this logic since isView semantics should not change.
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
Line 12614 (original), 12620 (patched)
<https://reviews.apache.org/r/72433/#comment309572>
Not sure why we need this change since information should be in qb.
- Jesús Camacho Rodríguez
On April 25, 2020, 11:23 a.m., Miklos Gergely wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72433/
> -----------------------------------------------------------
>
> (Updated April 25, 2020, 11:23 a.m.)
>
>
> Review request for hive and Jesús Camacho Rodríguez.
>
>
> Bugs: HIVE-23244
> https://issues.apache.org/jira/browse/HIVE-23244
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Create View commands are not queries, but commands which have queries as a part of them. Therefore a separate CreateViewAnalyzer is needed which uses SemanticAnalyer to analyze it's query.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 8e643fe844
> parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g b03b0989b8
> ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLUtils.java b82fc5e91d
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AbstractCreateViewAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AbstractCreateViewDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsOperation.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewOperation.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewDesc.java d1f36945fb
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java f7952a5cc1
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/ReplLoadTask.java b578d48ce1
> ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 4f1e23d7a6
> ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 7b2e201e5a
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java bef02176c2
> ql/src/java/org/apache/hadoop/hive/ql/parse/QB.java 9d94b6e2dd
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 0de3730351
> ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java 2350646c36
> ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 2f3fc6c50a
> ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java c75829c272
> ql/src/java/org/apache/hadoop/hive/ql/plan/LoadFileDesc.java 07bcef8ee3
> ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java 980f39b681
> ql/src/test/results/clientnegative/create_or_replace_view4.q.out 767cc77596
> ql/src/test/results/clientnegative/create_view_failure3.q.out 8b79272f46
> ql/src/test/results/clientnegative/create_view_failure5.q.out b7b3984292
> ql/src/test/results/clientnegative/create_view_failure6.q.out 6d9fb6461d
> ql/src/test/results/clientnegative/create_view_failure7.q.out 337dbe889e
> ql/src/test/results/clientnegative/create_view_failure8.q.out cccb7e4b06
> ql/src/test/results/clientnegative/create_view_failure9.q.out eac8cb489e
> ql/src/test/results/clientnegative/selectDistinctStarNeg_1.q.out 9496e528e2
> ql/src/test/results/clientpositive/create_view.q.out 7414d4749d
> ql/src/test/results/clientpositive/explain_ddl.q.out aea46d304f
> ql/src/test/results/clientpositive/llap/create_view_translate.q.out b5d464e716
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out 7f0ce5a9c7
> ql/src/test/results/clientpositive/llap/lineage3.q.out a4f6e61af4
> ql/src/test/results/clientpositive/llap/selectDistinctStar.q.out 3fc0074ed4
> ql/src/test/results/clientpositive/llap/union_top_level.q.out e0d86b3989
> ql/src/test/results/clientpositive/llap/vector_windowing.q.out ca3c6337bf
> ql/src/test/results/clientpositive/tez/explainanalyze_3.q.out dad999ab1a
> ql/src/test/results/clientpositive/tez/explainuser_3.q.out 5218c90f0c
>
>
> Diff: https://reviews.apache.org/r/72433/diff/1/
>
>
> Testing
> -------
>
> All the tests are still running fine.
>
>
> Thanks,
>
> Miklos Gergely
>
>
Re: Review Request 72433: Extract Create View analyzer from
SemanticAnalyzer
Posted by Miklos Gergely <mg...@hortonworks.com>.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java
> > Line 442 (original)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227713#file2227713line442>
> >
> > Why is this error code going away? Even if it is not used probably makes sense to keep it around and mark it as deprecated for backwards reference.
I agree, put it back, and added deprecation with explanation.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227721#file2227721line49>
> >
> > It seems more appropiate to update the display name too at this point.
I was planning to do that in the next patch, when I'm going to clean up materialized view creation, but it can be done here too. I've modified the patch.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java
> > Lines 67 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227721#file2227721line67>
> >
> > // only used for materialized views
> >
> > These comments can go away.
I was planning to do that in the next patch, when I'm going to clean up materialized view creation, but it can be done here too. I've modified the patch.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java
> > Line 1818 (original), 1818 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227727#file2227727line1818>
> >
> > Can we make this protected again? I did not see any usage that is not by a subclass?
Sure, it's protected again.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
> > Lines 566 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227728#file2227728line566>
> >
> > What is the goal of this block? Why is this needed now and it was not needed before? This seems more than a refactoring.
> >
> > Fwiw there is some logic below executed when we create a view in _handleCreateViewDDL_.
We needed this before as well, this is what replaces handleCreateViewDDL for view creation. Please see the explanation below in the next issue about the new paradigm. From now on the condition
cboCtx.type == PreCboCtx.Type.VIEW
will not be true for CREATE VIEW commands as the SA won't "know" that it is parsing a query for a view creation. I've tried to move all DDL related things into the DDL analyzer (CreateViewAnalyzer), right now these two commands found here had to stay here, but the rest of handleCreateViewDDL is now happening in the CreateViewAnalyzer.
Just to be clear, in the near future I'm planning to remove handleCreateViewDDL completely, once CREATE MATERIALIZED VIEW command will have their own analyzer too, and they will use SA only to analyze their query in a similar fashion.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
> > Line 1848 (original), 1854 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227728#file2227728line1854>
> >
> > Why are we changing this? We should not have to change this logic since isView semantics should not change.
isView was removed. When there is a CREATE VIEW command then it is parsed into an AST tree, which containes some info about the view (lets call it V) and a query (lets call it Q).
So far the whole tree was sent to the SemanticAnalyzer, kind of like saying that a CREATE VIEW command is just a special query. Then the SA identified that it is actually a CREATE VIEW command, so it created a CreateViewDesc for it, analyzing the V part, and saved it in the actual QB instance - no idea why there, I believe it just had to be put somewhere. Then the Q part was analyzed like every other query, and the results of this analysis were added to the CreateViewDesc, which could be always found in the QB.
After this change we'll have a new paradigm: a CREATE VIEW command is not a special query, it is a DDL, which contains a query. We'll have a CreateViewAnalyzer, which analyzes the V part, and as the Q part is a query, it sends the Q part, and only the Q part to a new SA. Ideally the SA would be 100% agnostic of what it is analyzing, whether it is a "regular" query, or a query of a CREATE VIEW command, but so far there are parts of the code which needs to know this. After this change it can't be done by looking at the QB if it contains a CreateViewDesc, but we still need this information, so I've introduced a forViewCreation global variable for now. I hope that in the future we'll find a way to rermove it, and make the SA totally agnostic of the source of the query, and then the forViewCreation will disappear too, but for now it is needed.
After this one is merged I'm planning to have a patch for removing the CREATE MATERIALIZED VIEW commands from the SA, then the getQB().isMaterializedView() command will disappear as well.
> On May 22, 2020, 10:18 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
> > Line 12614 (original), 12620 (patched)
> > <https://reviews.apache.org/r/72433/diff/1/?file=2227731#file2227731line12624>
> >
> > Not sure why we need this change since information should be in qb.
Same as above.
- Miklos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72433/#review220850
-----------------------------------------------------------
On June 3, 2020, 2:54 a.m., Miklos Gergely wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72433/
> -----------------------------------------------------------
>
> (Updated June 3, 2020, 2:54 a.m.)
>
>
> Review request for hive and Jesús Camacho Rodríguez.
>
>
> Bugs: HIVE-23244
> https://issues.apache.org/jira/browse/HIVE-23244
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Create View commands are not queries, but commands which have queries as a part of them. Therefore a separate CreateViewAnalyzer is needed which uses SemanticAnalyer to analyze it's query.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java d732004e51
> parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 768a3a17a7
> ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLUtils.java b82fc5e91d
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AbstractCreateViewAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AbstractCreateViewDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/AlterViewAsOperation.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewDesc.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateMaterializedViewOperation.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewAnalyzer.java PRE-CREATION
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewDesc.java d1f36945fb
> ql/src/java/org/apache/hadoop/hive/ql/ddl/view/create/CreateViewOperation.java f7952a5cc1
> ql/src/java/org/apache/hadoop/hive/ql/exec/repl/ReplLoadTask.java 792e331884
> ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 377e8280e5
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java da443f419c
> ql/src/java/org/apache/hadoop/hive/ql/parse/QB.java 9d94b6e2dd
> ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 8238a2a4a2
> ql/src/java/org/apache/hadoop/hive/ql/parse/StorageFormat.java 2350646c36
> ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 2f3fc6c50a
> ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java c75829c272
> ql/src/java/org/apache/hadoop/hive/ql/plan/LoadFileDesc.java 07bcef8ee3
> ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java 2fb452bea5
> ql/src/test/results/clientnegative/create_or_replace_view4.q.out 767cc77596
> ql/src/test/results/clientnegative/create_view_failure3.q.out 8b79272f46
> ql/src/test/results/clientnegative/create_view_failure5.q.out b7b3984292
> ql/src/test/results/clientnegative/create_view_failure6.q.out 6d9fb6461d
> ql/src/test/results/clientnegative/create_view_failure7.q.out 337dbe889e
> ql/src/test/results/clientnegative/create_view_failure8.q.out cccb7e4b06
> ql/src/test/results/clientnegative/create_view_failure9.q.out eac8cb489e
> ql/src/test/results/clientnegative/selectDistinctStarNeg_1.q.out 9496e528e2
> ql/src/test/results/clientpositive/llap/create_view.q.out 52b77c7505
> ql/src/test/results/clientpositive/llap/create_view_translate.q.out b5d464e716
> ql/src/test/results/clientpositive/llap/explain_ddl.q.out 3cada1bbae
> ql/src/test/results/clientpositive/llap/explainuser_1.q.out a2f241c470
> ql/src/test/results/clientpositive/llap/lineage3.q.out c87d7c0c92
> ql/src/test/results/clientpositive/llap/masking_mv.q.out 196688a17d
> ql/src/test/results/clientpositive/llap/materialized_view_cluster.q.out e34147d1da
> ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_3.q.out 6e6ee34361
> ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_4.q.out 23489244bd
> ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_rebuild_dummy.q.out d4bf2a6d8b
> ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_time_window.q.out dc0e861863
> ql/src/test/results/clientpositive/llap/materialized_view_distribute_sort.q.out cdcc356ae3
> ql/src/test/results/clientpositive/llap/materialized_view_partition_cluster.q.out 4d6de9d8f7
> ql/src/test/results/clientpositive/llap/materialized_view_partitioned.q.out e463b3cba6
> ql/src/test/results/clientpositive/llap/materialized_view_partitioned_3.q.out 8bdbf13d8a
> ql/src/test/results/clientpositive/llap/selectDistinctStar.q.out 3fc0074ed4
> ql/src/test/results/clientpositive/llap/union_top_level.q.out f846cb2fc1
> ql/src/test/results/clientpositive/llap/vector_windowing.q.out ba31832201
> ql/src/test/results/clientpositive/tez/explainanalyze_3.q.out 33546dd74f
> ql/src/test/results/clientpositive/tez/explainuser_3.q.out f1c245b671
>
>
> Diff: https://reviews.apache.org/r/72433/diff/3/
>
>
> Testing
> -------
>
> All the tests are still running fine.
>
>
> Thanks,
>
> Miklos Gergely
>
>