> On 2011-07-28 21:40:30, John Sichi wrote: > > ql/src/java/org/apache/hadoop/hive/ql/index/AggregateIndexHandler.java, > > line 61 > > <https://reviews.apache.org/r/1194/diff/1/?file=27052#file27052line61> > > > > Please run ant checkstyle and fix all the formatting discrepancies it > > reports for your new files. > > > > Prajakta Kalmegh wrote: > Done! The code is still having checkstyle formatting errors only for > places where we have used LinkedHashMap, HashMap and ArrayList. The error > states "Declaring variables, return values or parameters of type 'HashMap' is > not allowed".
Best practice is to only use interfaces (Map/List) except at the point of instantiation where you select a concrete class. Hive violates this in a number of places, and sometimes that forces you to violate it in new code too; but otherwise, please follow this one. > On 2011-07-28 21:40:30, John Sichi wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java, line > > 603 > > <https://reviews.apache.org/r/1194/diff/1/?file=27062#file27062line603> > > > > Not sure why this new constructor is needed...after using it, all you > > do is get the table out of it. > > Prajakta Kalmegh wrote: > The only other constructor option for tableSpec needs the ASTNode as one > of its parameters. Since we need to construct a new tableSpec using only the > index table name, and we do not have a ASTNode for this, I need this > constructor. If you have any other way in mind, please let me know. That > would be helpful. I'm asking why you even need to construct a new tableSpec instance. All you do with it is reference ts.tableHandle. And to create that tableHandle, you can just do db.getTable(tableName). So I don't see the purpose of the tableSpec instance. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1194/#review1212 ----------------------------------------------------------- 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 > >
