> 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 ?
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.
> 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."
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.
- Amareshwari
-----------------------------------------------------------
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
>
>
