> 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.
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. > 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 328 (patched) > > <https://reviews.apache.org/r/69663/diff/1/?file=2117432#file2117432line332> > > > > Can we move this method as _invertFunction_ utility method to > > _FunctionRegistry.java_? > > > > In addition, instead of relying on the function text, I believe it > > would be more robust to have the UDF as the input. In particular, we can > > use _funcDesc.getGenericUDF();_ when calling this method, then rely in e.g. > > _udf instanceof GenericUDFOPEqualOrGreaterThan_ for the checks. > > Deepak Jaiswal wrote: > Yes, I can move this. > The reason I used function text is because I can use switch case and also > much faster. > Otherwise, once extended in future, this could become a giant mess of > if...else statements. > We can discuss this further. I was thinking about the function UDFs because it is less shaky than text matching, but on the other hand, it is not probable that Hive ever changes the string representation for these functions and as you said it is simpler, hence it should be fine. > 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. 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)? - Jesús ----------------------------------------------------------- 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 > >
