----------------------------------------------------------- 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 > >
