> On Dec. 26, 2014, 3:46 p.m., Amareshwari Sriramadasu wrote: > > lens-cube/src/main/java/org/apache/lens/cube/parse/GroupbyResolver.java, > > line 197 > > <https://reviews.apache.org/r/29422/diff/3/?file=801333#file801333line197> > > > > There wont be any ASTNode passed here which is not select expr node, > > right? Is this if check required? > > > > Also the methods isSelectExprASTNode in HQLParser > > Himanshu Gahlaut wrote: > If we are sure that a SELECT ASTNode can only have SELET_EXPR AST Nodes > as children, then we can remove this check here. Is this declared in the > contract of Hive Parser that SELECT AST Node will only have SELECT_EXPR as > children ? > > Amareshwari Sriramadasu wrote: > Here is the antlr grammar corresponding to select clause (copied from > SelectClauseParser.g from hive code): > > selectClause > @init { gParent.pushMsg("select clause", state); } > @after { gParent.popMsg(state); } > : > KW_SELECT hintClause? (((KW_ALL | dist=KW_DISTINCT)? selectList) > | (transform=KW_TRANSFORM selectTrfmClause)) > -> {$transform == null && $dist == null}? ^(TOK_SELECT hintClause? > selectList) > -> {$transform == null && $dist != null}? ^(TOK_SELECTDI hintClause? > selectList) > -> ^(TOK_SELECT hintClause? ^(TOK_SELEXPR selectTrfmClause) ) > | > trfmClause ->^(TOK_SELECT ^(TOK_SELEXPR trfmClause)) > ; > > selectList > @init { gParent.pushMsg("select list", state); } > @after { gParent.popMsg(state); } > : > selectItem ( COMMA selectItem )* -> selectItem+ > ; > > selectItem > @init { gParent.pushMsg("selection target", state); } > @after { gParent.popMsg(state); } > : > ( selectExpression > ((KW_AS? identifier) | (KW_AS LPAREN identifier (COMMA identifier)* > RPAREN))? > ) -> ^(TOK_SELEXPR selectExpression identifier*) > ; > > You can add appropriate javadoc about the assumptions. We are not > allowing hints and KW_ALL in cube HQL. So, all children will be Select expr > AST nodes.
Sure will make this change and put a comment for assumption. > On Dec. 26, 2014, 3:46 p.m., Amareshwari Sriramadasu wrote: > > lens-cube/src/main/java/org/apache/lens/cube/parse/GroupbyResolver.java, > > line 219 > > <https://reviews.apache.org/r/29422/diff/3/?file=801333#file801333line219> > > > > Is there any node which is not SelectASTNode node? > > then is the if check required? Then the method > > isSelectASTNode in HQLParser required? > > Himanshu Gahlaut wrote: > A method normally has following stages: > > 1) Precondition > 2) Logic (Assumes precondtion is met) > 3) Postconditions (cleanup) > 4) Return > > The Logic of filterNonMsrNonAggSelectASTChildren assumes that a > precondition is met by node input into the function. This check is to ensure > the precondition. Currently the method is trying to give expected results and > is returning an empty list when input node is not a select AST Node. If we > remove this check, than we'll have to put following comment in javadoc of > this method: "If input node is not a select ASTNode then return value is > unexpected." > > Amareshwari Sriramadasu wrote: > Sure. Since the method is private, I'm thinking putting the javadoc with > respect to expected results to good enough, than to add more code. As far as API contracts are concerned, viewing private and public method with the same eye is in general beneficial. The moment a developer decides to make this method public, there are more chances of him/her missing the java doc. This will lead to code which will be good for now but will be easier to go bad in future. - Himanshu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29422/#review66156 ----------------------------------------------------------- On Dec. 27, 2014, 10:55 a.m., Himanshu Gahlaut wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29422/ > ----------------------------------------------------------- > > (Updated Dec. 27, 2014, 10:55 a.m.) > > > Review request for lens. > > > Repository: lens > > > Description > ------- > > (1) getExpressionWithoutAlias in GroupbyResolver.class is unable to handle > aliases with spaces in them. Fixed the same. > (2) Also improved log4j.properties of lens-cube to send logs to both console > appender and test log files. > (3) Used Slf4j annotation in Test classes for getting a reference to Slf4j > logger. Added required dependencies in lens-cube/pom.xml to facilitate the > same. > > > Diffs > ----- > > lens-cube/pom.xml 1bcb4dc > lens-cube/src/main/java/org/apache/lens/cube/parse/GroupbyResolver.java > c2fef7e > lens-cube/src/main/java/org/apache/lens/cube/parse/HQLParser.java 8e41830 > lens-cube/src/test/java/org/apache/lens/cube/parse/CubeTestSetup.java > f515df1 > lens-cube/src/test/java/org/apache/lens/cube/parse/TestCubeRewriter.java > ad4abcf > lens-cube/src/test/java/org/apache/lens/cube/parse/TestQueryRewrite.java > fd939e9 > lens-cube/src/test/resources/log4j.properties 9729de0 > > Diff: https://reviews.apache.org/r/29422/diff/ > > > Testing > ------- > > Added new unit test cases for the changes. > All existing Unit Test Cases passed. > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] Lens Checkstyle Rules ............................. SUCCESS [14.068s] > [INFO] Lens .............................................. SUCCESS [5.717s] > [INFO] Lens API .......................................... SUCCESS [9.369s] > [INFO] Lens API for server and extensions ................ SUCCESS [8.624s] > [INFO] Lens Cube ......................................... SUCCESS [7:10.440s] > [INFO] Lens DB storage ................................... SUCCESS [23.631s] > [INFO] Lens Query Library ................................ SUCCESS [9.386s] > [INFO] Lens Hive Driver .................................. SUCCESS [3:34.922s] > [INFO] Lens Driver for Cloudera Impala ................... SUCCESS [10.798s] > [INFO] Lens Driver for JDBC .............................. SUCCESS [36.429s] > [INFO] Lens Server ....................................... SUCCESS > [14:29.334s] > [INFO] Lens client ....................................... SUCCESS [45.711s] > [INFO] Lens CLI .......................................... SUCCESS [3:10.102s] > [INFO] Lens Examples ..................................... SUCCESS [3.064s] > [INFO] Lens Distribution ................................. SUCCESS [11.315s] > [INFO] Lens Client Distribution .......................... SUCCESS [7.496s] > [INFO] Lens ML Lib ....................................... SUCCESS [2:32.231s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 34:03.600s > [INFO] Finished at: Fri Dec 26 18:21:01 IST 2014 > [INFO] Final Memory: 100M/332M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Himanshu Gahlaut > >
