> On June 20, 2017, 12:17 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java > > Lines 137-139 (patched) > > <https://reviews.apache.org/r/59984/diff/1/?file=1747770#file1747770line137> > > > > Seems like this if branch is unnecessary. > > Vineet Garg wrote: > I don't think so, most of the calcite code/ hive rules implement > RelShuttle interface. So accept should reroute accordingly
Its unnecessary in sense that even without this branch logic of this function doesn't change. > On June 20, 2017, 12:17 a.m., Ashutosh Chauhan wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelDecorrelator.java > > Lines 3088 (patched) > > <https://reviews.apache.org/r/59984/diff/1/?file=1747773#file1747773line3109> > > > > Might be better to initialize this variable to true. Since, value being > > false is known and limited set. > > Vineet Garg wrote: > I think this works better for readability. e.g. Aggregate looks for > grouping sets and if it finds one sets the flag to true and return, otherwise > it traveses all its children. Changing the intial flag to true will entail > negating this logic and will anyway keep all the code/cases. Initialization with false is more robust since that implies by default valueGen is required. Thats a good thing since we know all cases when its not required, but not necessarily otherwise. e.g., if there are more visit methods added in this interface in future and this implementation is not updated, initialization with false will save us from bugs like those. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59984/#review178248 ----------------------------------------------------------- On June 11, 2017, 7:17 p.m., Vineet Garg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59984/ > ----------------------------------------------------------- > > (Updated June 11, 2017, 7:17 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-16838 > https://issues.apache.org/jira/browse/HIVE-16838 > > > Repository: hive-git > > > Description > ------- > > This patch improves plans for subqueries which have not equal corelated > predicates. > Currently to retrieve all possible correlated predicates inner table is > joined with outer query. This is un-necessary in most of the cases (exception > is if subquery has an aggregate). > > > Diffs > ----- > > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java > 63bbdaccfb > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveIntersect.java > 19e1e026f4 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveUnion.java > 7cfb007a9d > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelDecorrelator.java > 4c99932759 > ql/src/test/queries/clientpositive/subquery_in.q 4ba170a706 > ql/src/test/results/clientpositive/constprog_partitioner.q.out 8c7f9d3f29 > ql/src/test/results/clientpositive/llap/explainuser_1.q.out 8b04bc9261 > ql/src/test/results/clientpositive/llap/subquery_exists.q.out 3004e36c9d > ql/src/test/results/clientpositive/llap/subquery_in.q.out 1f9c9e4474 > ql/src/test/results/clientpositive/llap/subquery_multi.q.out 29516eff82 > ql/src/test/results/clientpositive/llap/subquery_notin.q.out b4af91579b > ql/src/test/results/clientpositive/llap/subquery_scalar.q.out b78df8b9f5 > ql/src/test/results/clientpositive/llap/subquery_select.q.out 202980e975 > ql/src/test/results/clientpositive/llap/subquery_views.q.out 1a21a02a30 > ql/src/test/results/clientpositive/llap/vector_mapjoin_reduce.q.out > d3586e0db2 > ql/src/test/results/clientpositive/perf/query16.q.out a7f93f9ec2 > ql/src/test/results/clientpositive/perf/query94.q.out c5fc9e7f00 > ql/src/test/results/clientpositive/spark/constprog_partitioner.q.out > 3467215d63 > ql/src/test/results/clientpositive/spark/subquery_exists.q.out 8768b45166 > ql/src/test/results/clientpositive/spark/subquery_in.q.out 80a350656d > ql/src/test/results/clientpositive/spark/vector_mapjoin_reduce.q.out > 2f2609f03e > ql/src/test/results/clientpositive/subquery_exists.q.out cfc76520ce > ql/src/test/results/clientpositive/subquery_exists_having.q.out 2c41ff6c33 > ql/src/test/results/clientpositive/subquery_in_having.q.out c4569ba035 > ql/src/test/results/clientpositive/subquery_notexists.q.out 039df03819 > ql/src/test/results/clientpositive/subquery_notexists_having.q.out > fda801d387 > ql/src/test/results/clientpositive/subquery_notin_having.q.out 462dda5e14 > ql/src/test/results/clientpositive/subquery_unqualcolumnrefs.q.out > 03eb4b6ba4 > ql/src/test/results/clientpositive/vector_mapjoin_reduce.q.out 82bef24230 > > > Diff: https://reviews.apache.org/r/59984/diff/1/ > > > Testing > ------- > > * Added new tests > * Pre-commit testing > > > Thanks, > > Vineet Garg > >
