hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jesús Camacho Rodríguez <jcamachorodrig...@hortonworks.com>
Subject Re: Review Request 40101: CBO: Calcite Operator To Hive Operator (Calcite Return Path): fix test failure for udf_percentile.q
Date Mon, 30 Nov 2015 02:16:41 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40101/#review105902
-----------------------------------------------------------



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java (line
1019)
<https://reviews.apache.org/r/40101/#comment167622>

    Lines 1019-1026.
    
    We store the position of the argument for the function in the input (_argPos_). But then
the logic is difficult to follow.
    _distinctPositions_ stores _argPos_, but _argPos_ is not used to obtain _rsUDAFParamColInfo_
from _rsColInfoLst_ in the _else_ block. Why? Does the code make the assumption that we visit
the distinct positions in order i.e. pos 3 should always have been visited before 4?
    Thus, in the _else_ block, _gbInfo.gbKeys.size() + distinctPositions.size()_ = _argPos_,
right? Then the _rsColInfoLst.get_ call could be taken out of the block.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java (line
1043)
<https://reviews.apache.org/r/40101/#comment167623>

    Can we take the call out of the _j_ for loop i.e. moved to the outer loop?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java (line
1058)
<https://reviews.apache.org/r/40101/#comment167626>

    If the gby schema is --grpkey--,--distkey--,--values-- and we are trying to shift, would
it be enough to sum up _distinctPositions.size_ and _argPos_ iff _argPos >= gbKeys.size()_,
otherwise use _argPos_ ?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java (line
1069)
<https://reviews.apache.org/r/40101/#comment167624>

    Can we take the call out of the _j_ for loop i.e. moved to the outer loop?


- Jesús Camacho Rodríguez


On Nov. 10, 2015, 12:58 a.m., pengcheng xiong wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40101/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2015, 12:58 a.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The position in argList is mapped to a wrong column from RS operator
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java
a129cf3 
>   ql/src/test/queries/clientpositive/cbo_rp_groupby3_noskew_multi_distinct.q PRE-CREATION

>   ql/src/test/queries/clientpositive/cbo_rp_udf_percentile.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/cbo_rp_udf_percentile2.q PRE-CREATION 
>   ql/src/test/results/clientpositive/cbo_rp_groupby3_noskew_multi_distinct.q.out PRE-CREATION

>   ql/src/test/results/clientpositive/cbo_rp_udf_percentile.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/cbo_rp_udf_percentile2.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40101/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> pengcheng xiong
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message