> 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)? > > Deepak Jaiswal wrote: > 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. > > Jesús Camacho Rodríguez wrote: > OK, I was getting confused by the semantics of the srcPos parameter (an > 'invert' boolean would have been clearer). > Tbh, I think it is better to create two methods: one internal in > SyntheticJoinPredicate that would return whether a function is supported or > not, and a utility method in FunctionRegistry that would return the inverse > of a given function. Overhead is neglibible and there will be clear different > semantics.
Having two functions could create a maintenance headache. As mentioned above, the function will go to FunctionRegistry. There is already a comment, return null; // helps identify unsupported functions I can expand the comment to make things clearer. Leaving the function as it is keeps things short and sweet and involves much less maintenance. - 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 > >
