> On Jan. 7, 2019, 4:41 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java > > Line 182 (original) > > <https://reviews.apache.org/r/69663/diff/1/?file=2117432#file2117432line183> > > > > Can you bring this back and delete 'if (sourceKeys.size() > 0) {' > > below? This is just a style change and indenting so many lines will just > > make more difficult following code provenance. > > Deepak Jaiswal wrote: > The continue is removed so that it reaches the residualFilter logic, > otherwise it would skip everything and move on to next target. > > Jesús Camacho Rodríguez wrote: > You are right, I did not see the extra }. Could the comment '//if > (sourceKeys.size() < 1) continue;' below be removed then? No need to leave it > there.
Sure. I forgot to remove it. > On Jan. 7, 2019, 4:41 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java > > Lines 330 (patched) > > <https://reviews.apache.org/r/69663/diff/1/?file=2117432#file2117432line334> > > > > Should we still return null if function text is not recognized? > > Deepak Jaiswal wrote: > Yes, that helps recognize unsupported functions. For eg, > > ExprNodeGenericFuncDesc funcDesc = (ExprNodeGenericFuncDesc) > filter; > // filter should be of type <, >, <= or >= > if (getFuncText(funcDesc.getFuncText(), 1) == null) { > // unsupported > continue; > } > > I am open to better ways, hence the TODO. > > Jesús Camacho Rodríguez wrote: > Sorry, I did not express myself properly. Within the if (srcPos == 0) {, > shouldn't we return null if function text is not recognized (similar to what > we do below that you pointed out)? That would require verifying the function text which is done in switch case anyway. Inorder for non-equi join's synthetic joins to work properly, if the switch case cant get a valid inversion text then it is not supported. That is why I used "1" to make sure it goes through the switch case. This eliminates duplicating similar logic. - Deepak ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69663/#review211725 ----------------------------------------------------------- On Jan. 3, 2019, 8:39 p.m., Deepak Jaiswal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69663/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2019, 8:39 p.m.) > > > Review request for hive, Ashutosh Chauhan, Gopal V, Jesús Camacho Rodríguez, > and Jason Dere. > > > Bugs: HIVE-16976 > https://issues.apache.org/jira/browse/HIVE-16976 > > > Repository: hive-git > > > Description > ------- > > DPP: SyntheticJoinPredicate transitivity for < > and BETWEEN > > The patch supports predicates on non-equi joins and provides an interface for > storage handler to decide if it can use this optimization. > Work to integrate this with DPP and semijoin will be done in separate JIRA. > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageHandler.java > 2ebb149354 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/DynamicPartitionPruningOptimization.java > a1401aac72 > ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java f8c7e18eb1 > ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDynamicListDesc.java > 676dfc9421 > ql/src/java/org/apache/hadoop/hive/ql/ppd/SyntheticJoinPredicate.java > e97e44796f > ql/src/test/results/clientpositive/llap/cross_prod_1.q.out ac1f4eabd8 > ql/src/test/results/clientpositive/llap/groupby_groupingset_bug.q.out > de74af6dff > ql/src/test/results/clientpositive/llap/semijoin.q.out 63a270e57d > ql/src/test/results/clientpositive/llap/subquery_in.q.out 07cc4dbabc > ql/src/test/results/clientpositive/llap/subquery_notin.q.out 29d8bbfb48 > ql/src/test/results/clientpositive/llap/subquery_scalar.q.out e830835445 > ql/src/test/results/clientpositive/llap/subquery_select.q.out d3cc980ca1 > > > Diff: https://reviews.apache.org/r/69663/diff/1/ > > > Testing > ------- > > > Thanks, > > Deepak Jaiswal > >
