----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34399/#review85122 -----------------------------------------------------------
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateFact.java <https://reviews.apache.org/r/34399/#comment136602> @Getter lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java <https://reviews.apache.org/r/34399/#comment136603> We can refactor it to three functions returning their own `toRemove`. Then in the original function: `if(a() or b() or c()) { iter.remove }` lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java <https://reviews.apache.org/r/34399/#comment136604> first `updateEvaluable` then `isEvaluable` with exactly same arguments. Seems weird. lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136646> I don't think a blank line is needed. lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136598> `exprDimensions` ? lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136647> or size zero lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136599> `isHasMeasures()` -> `hasMeasures()` lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136648> Both if and else (over split.length) are doing the same thing with different arguments. lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java <https://reviews.apache.org/r/34399/#comment136649> Let's also call this `isCubeMeasure` since it's returning the result of `isCubeMeasure` and doing essencially the same thing. Having different names makes it confusing to understand and use by callers. lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java <https://reviews.apache.org/r/34399/#comment136650> Can't we merge the two for loops? lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java <https://reviews.apache.org/r/34399/#comment136651> passing both `cubeql` and `cubeql.something()`. Let's pass only `cubeql`. lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java <https://reviews.apache.org/r/34399/#comment136653> use getter. `esc.getTblAliasToColumns()` lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java <https://reviews.apache.org/r/34399/#comment136652> commented? lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java <https://reviews.apache.org/r/34399/#comment136666> Unit test for this? lens-cube/src/main/java/org/apache/lens/cube/parse/TrackQueriedColumns.java <https://reviews.apache.org/r/34399/#comment136654> public? lens-cube/src/test/resources/log4j.properties <https://reviews.apache.org/r/34399/#comment136655> Why is this changed? - Rajat Khandelwal On May 22, 2015, 1:24 p.m., Amareshwari Sriramadasu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34399/ > ----------------------------------------------------------- > > (Updated May 22, 2015, 1:24 p.m.) > > > Review request for lens, Jaideep dhok and Rajat Khandelwal. > > > Bugs: LENS-174 > https://issues.apache.org/jira/browse/LENS-174 > > > Repository: lens > > > Description > ------- > > Changes are the following : > > - Fix Dimension.getColumnByName() to return expression column as well. This > was existing bug, got fixed now > - Adds ExpressionContext (new inner class) for each expression queried in > ExpressionResolver. And > - ExpressionContext is updated with ExpressionSpecContext (new inner class) > for each expression in expressioncolumn. It adds newer ExpressionSpecContext > by expanding all nested expressions. > - Some methods in AliasReplacer and ColumnResolver are made static sothat > they can called from ExpressionResolver as well. > - AggregateResolver wraps appropriate aggregate around fields in expressions, > along with ASTs > - CandidateTableResolver picks all facts if expression column is directly > available. if not, it checks if the expression is evaluatable by the > candidate table. > - Finally CubeQueryContext.toHQL() picks appropriate expressions for > candidates picked and replaces them in AST. A picked expression can add more > tables to joined. > - FieldValidator to validate derived cube corresponding to expression along > other fields qeuried > - TimerangeResolver to remove expressions which not valid in the range > - Add more testcases to cover cornercases > - Allow expressions to have denorm columns and added tests > > > Diffs > ----- > > lens-cube/src/main/java/org/apache/lens/cube/metadata/Dimension.java > 1a4b581 > lens-cube/src/main/java/org/apache/lens/cube/metadata/ExprColumn.java > a7f711f > lens-cube/src/main/java/org/apache/lens/cube/parse/AggregateResolver.java > 76b5729 > lens-cube/src/main/java/org/apache/lens/cube/parse/AliasReplacer.java > 9d367c3 > lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateFact.java > 52bf9aa > > lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java > 8c009b2 > > lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java > 40561ad > lens-cube/src/main/java/org/apache/lens/cube/parse/ColumnResolver.java > 1aa33db > lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java > 3964c1a > lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryRewriter.java > b7a92e7 > > lens-cube/src/main/java/org/apache/lens/cube/parse/DenormalizationResolver.java > e0f7bea > lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java > 5355049 > lens-cube/src/main/java/org/apache/lens/cube/parse/FieldValidator.java > fd8568f > lens-cube/src/main/java/org/apache/lens/cube/parse/JoinResolver.java > 17d2eed > lens-cube/src/main/java/org/apache/lens/cube/parse/TimerangeResolver.java > 936faa1 > lens-cube/src/main/java/org/apache/lens/cube/parse/TrackQueriedColumns.java > PRE-CREATION > lens-cube/src/test/java/org/apache/lens/cube/parse/CubeTestSetup.java > 5c776cc > > lens-cube/src/test/java/org/apache/lens/cube/parse/FieldsCannotBeQueriedTogetherTest.java > 1e0dbf8 > lens-cube/src/test/java/org/apache/lens/cube/parse/TestBaseCubeQueries.java > 2fd0a46 > > lens-cube/src/test/java/org/apache/lens/cube/parse/TestDenormalizationResolver.java > 71ed1a8 > > lens-cube/src/test/java/org/apache/lens/cube/parse/TestExpressionContext.java > PRE-CREATION > > lens-cube/src/test/java/org/apache/lens/cube/parse/TestExpressionResolver.java > a78bcbd > lens-cube/src/test/java/org/apache/lens/cube/parse/TestJoinResolver.java > 2c8dab3 > lens-cube/src/test/java/org/apache/lens/cube/parse/TestQueryMetrics.java > 4304f94 > lens-cube/src/test/resources/log4j.properties f514648 > > Diff: https://reviews.apache.org/r/34399/diff/ > > > Testing > ------- > > - Added tests for picking different expressions of expression column > - Added tests for picking materialized expression value soemtimes and > expression sometimes > - Added tetss testing all nested expressions are added > - Added tests FieldNotQueryableTogether to test for expression fields. > > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] Lens Checkstyle Rules ............................. SUCCESS [3.214s] > [INFO] Lens .............................................. SUCCESS [3.542s] > [INFO] Lens API .......................................... SUCCESS [20.894s] > [INFO] Lens API for server and extensions ................ SUCCESS [18.985s] > [INFO] Lens Cube ......................................... SUCCESS [3:34.331s] > [INFO] Lens DB storage ................................... SUCCESS [20.569s] > [INFO] Lens Query Library ................................ SUCCESS [16.552s] > [INFO] Lens Hive Driver .................................. SUCCESS [2:56.814s] > [INFO] Lens Driver for JDBC .............................. SUCCESS [47.560s] > [INFO] Lens Server ....................................... SUCCESS [6:54.878s] > [INFO] Lens client ....................................... SUCCESS [44.291s] > [INFO] Lens CLI .......................................... SUCCESS [3:36.281s] > [INFO] Lens Examples ..................................... SUCCESS [11.696s] > [INFO] Lens Distribution ................................. SUCCESS [8.245s] > [INFO] Lens ML Lib ....................................... SUCCESS [1:19.084s] > [INFO] Lens ML Ext Distribution .......................... SUCCESS [1.708s] > [INFO] Lens Regression ................................... SUCCESS [8.307s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 21:47.838s > [INFO] Finished at: Fri May 22 07:10:12 UTC 2015 > [INFO] Final Memory: 165M/1275M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Amareshwari Sriramadasu > >
