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