----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51755/#review148788 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java (line 209) <https://reviews.apache.org/r/51755/#comment216301> Error message: Union or intersect ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java (line 30) <https://reviews.apache.org/r/51755/#comment220241> whitespace ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 31) <https://reviews.apache.org/r/51755/#comment216303> It will be good to add a comment describing transformation we are trying to achieve here. Better will be in terms of sql level transform, if possible. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 59) <https://reviews.apache.org/r/51755/#comment216304> Is this check not required on bottomIntersect? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java (line 65) <https://reviews.apache.org/r/51755/#comment216306> This assert should be before if() ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 64) <https://reviews.apache.org/r/51755/#comment220242> Good to add more details on how this rule is transforming operator tree. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 102) <https://reviews.apache.org/r/51755/#comment220243> Will be useful to explain why Project is needed here. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 107) <https://reviews.apache.org/r/51755/#comment220244> Remove TODO ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 108) <https://reviews.apache.org/r/51755/#comment220245> rethrow after logging. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 148) <https://reviews.apache.org/r/51755/#comment220246> Add comments on why we need to have project here which is not doing any transformations. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 150) <https://reviews.apache.org/r/51755/#comment220247> TODO ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 151) <https://reviews.apache.org/r/51755/#comment220248> log and rethrow. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 180) <https://reviews.apache.org/r/51755/#comment220249> It is a deterministic function. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 189) <https://reviews.apache.org/r/51755/#comment220250> TODO ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 190) <https://reviews.apache.org/r/51755/#comment220251> log and rethrow. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 212) <https://reviews.apache.org/r/51755/#comment220252> remove TODO ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java (line 213) <https://reviews.apache.org/r/51755/#comment220253> log and rethrow. ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java (line 2090) <https://reviews.apache.org/r/51755/#comment220254> ws ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g (line 93) <https://reviews.apache.org/r/51755/#comment220255> Good to call this TOK_EXCEPTALL since KW is except not minus. ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g (line 94) <https://reviews.apache.org/r/51755/#comment220258> TOK_EXCEPTDISTINCT ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java (line 516) <https://reviews.apache.org/r/51755/#comment220256> insideView not needed anymore? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java (line 523) <https://reviews.apache.org/r/51755/#comment220257> insideView not needed anymore? ql/src/test/queries/clientpositive/intersect.q (line 40) <https://reviews.apache.org/r/51755/#comment220259> Add a test where there is a GBy on different columns with different aggregates on two branches of intersect. - Ashutosh Chauhan On Sept. 9, 2016, 1:41 a.m., pengcheng xiong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51755/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2016, 1:41 a.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Repository: hive-git > > > Description > ------- > > HIVE-12765 > > > Diffs > ----- > > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java > cf93ed8 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectMergeRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveIntersectRewriteRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java > 9f5e733 > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java ff94160 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g 7ceb005 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g df596ff > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 9ba1865 > ql/src/java/org/apache/hadoop/hive/ql/parse/QBExpr.java cccf0f6 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 943d9d7 > ql/src/test/queries/clientpositive/intersect.q PRE-CREATION > ql/src/test/results/clientpositive/intersect.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/51755/diff/ > > > Testing > ------- > > > Thanks, > > pengcheng xiong > >
