> On Jan. 7, 2019, 4:41 p.m., Jesús Camacho Rodríguez wrote: > > Can we add some tests for the new feature?
The reason there is no test yet is because it does nothing end to end. Both DPP route and semijoin reduction route dont process the predicate yet. > 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. The continue is removed so that it reaches the residualFilter logic, otherwise it would skip everything and move on to next target. > 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 284 (patched) > > <https://reviews.apache.org/r/69663/diff/1/?file=2117432#file2117432line288> > > > > We should add a call to extended version here as we did above for > > equality predicates. The only required change seems to be in > > _addParentReduceSink_ called from _createDerivatives_, which would receive > > the comparison operator from here. All the rest should already work as > > expected. > > > > I believe this could be addressed in this JIRA since it is not a lot of > > code. However, if it is not addressed, please create follow-up and leave a > > TODO. Will add the extended version. Thanks for bringing this up. > 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 318 (patched) > > <https://reviews.apache.org/r/69663/diff/1/?file=2117432#file2117432line322> > > > > return colExprMap.get(rsColName) :| > 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. 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. > 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? 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. - 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 > >
