hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sichi" <jsi...@fb.com>
Subject Re: Review Request: HIVE-1694: Accelerate GROUP BY execution using indexes
Date Thu, 28 Jul 2011 21:40:30 GMT

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



ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java
<https://reviews.apache.org/r/1194/#comment2711>

    Please run ant checkstyle and fix all the formatting discrepancies it reports for your
new files.
    



ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java
<https://reviews.apache.org/r/1194/#comment2695>

    Don't you need to reuse the compact implementation here so that the index can be used
for WHERE (not just GROUP BY)?
    



ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
<https://reviews.apache.org/r/1194/#comment2696>

    This method is redundant now.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteCanApplyCtx.java
<https://reviews.apache.org/r/1194/#comment2698>

    I can't think of a case where it would be worse.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteCanApplyProcFactory.java
<https://reviews.apache.org/r/1194/#comment2699>

    Actually group-by is now preserved in all cases.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteGBUsingIndex.java
<https://reviews.apache.org/r/1194/#comment2700>

    Please use HTML bullet syntax for Javadoc (otherwise it all gets run together into one
line when rendered).
    



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteGBUsingIndex.java
<https://reviews.apache.org/r/1194/#comment2701>

    indentation



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteQueryUsingAggregateIndex.java
<https://reviews.apache.org/r/1194/#comment2703>

    Shouldn't this be BIGINT?
    
    Also, I think you're supposed to use a TypeInfoFactory for this purpose.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteQueryUsingAggregateIndex.java
<https://reviews.apache.org/r/1194/#comment2702>

    indentation



ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteQueryUsingAggregateIndex.java
<https://reviews.apache.org/r/1194/#comment2704>

    typo:  Repace



ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java
<https://reviews.apache.org/r/1194/#comment2707>

    Not sure why this new constructor is needed...after using it, all you do is get the table
out of it.



ql/src/test/queries/clientpositive/ql_rewrite_gbtoidx.q
<https://reviews.apache.org/r/1194/#comment2709>

    This should *not* be using the index, since the index is built on count(l_shipdate), and
l_shipdate may contain nulls, whereas the query is referencing count(1), which is insensitive
to nulls.



ql/src/test/queries/clientpositive/ql_rewrite_gbtoidx.q
<https://reviews.apache.org/r/1194/#comment2710>

    Need additional tests to verify all the cases where the optimization should *not* be used:
    
    * when configuration disables it
    * when index partitions do not cover table partitions (I still don't see the code for
this case)
    * ... all the other conditions checked for in the code ...
    


- John


On 2011-07-26 14:44:01, Prajakta Kalmegh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1194/
> -----------------------------------------------------------
> 
> (Updated 2011-07-26 14:44:01)
> 
> 
> Review request for hive and John Sichi.
> 
> 
> Summary
> -------
> 
> This patch has defined a new AggregateIndexHandler which is used to optimize the query
plan for groupby queries. 
> 
> 
> This addresses bug HIVE-1694.
>     https://issues.apache.org/jira/browse/HIVE-1694
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java b46976f 
>   ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java PRE-CREATION

>   ql/src/java/org/apache/hadoop/hive/ql/index/HiveIndex.java 591c9ff 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 2ca63b3 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java 590d69a 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteCanApplyCtx.java PRE-CREATION

>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteCanApplyProcFactory.java
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteGBUsingIndex.java PRE-CREATION

>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteParseContextGenerator.java
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteQueryUsingAggregateIndex.java
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/index/RewriteQueryUsingAggregateIndexCtx.java
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 77a6dc6 
>   ql/src/test/queries/clientpositive/ql_rewrite_gbtoidx.q PRE-CREATION 
>   ql/src/test/results/clientpositive/ql_rewrite_gbtoidx.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/1194/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Prajakta
> 
>


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