----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52381/#review151478 -----------------------------------------------------------
Fix it, then Ship it! Minor comments. lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java (line 345) <https://reviews.apache.org/r/52381/#comment220074> Can be made static lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java (line 744) <https://reviews.apache.org/r/52381/#comment219888> Can we add null check on `colSet` like the previous function? lens-cube/src/main/java/org/apache/lens/cube/parse/ColumnResolver.java (line 221) <https://reviews.apache.org/r/52381/#comment220075> isn't `alias!=null` always true inside this block? lens-cube/src/main/java/org/apache/lens/cube/parse/GroupbyResolver.java (lines 158 - 161) <https://reviews.apache.org/r/52381/#comment220076> Can we change names of the `getExpression` methods? Two different functions with a generic name seems a bit confusing. lens-cube/src/test/java/org/apache/lens/cube/parse/TestBaseCubeQueries.java (line 90) <https://reviews.apache.org/r/52381/#comment219896> Can we also add assert on the column absent from all facts? lens-cube/src/test/java/org/apache/lens/cube/parse/TestBaseCubeQueries.java (line 744) <https://reviews.apache.org/r/52381/#comment219897> Is join happening on dim22 also? - Rajat Khandelwal On Sept. 29, 2016, 3:19 p.m., Amareshwari Sriramadasu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52381/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2016, 3:19 p.m.) > > > Review request for lens and Rajat Khandelwal. > > > Bugs: LENS-1273 > https://issues.apache.org/jira/browse/LENS-1273 > > > Repository: lens > > > Description > ------- > > The problem exists with case when aggregate expressions in current code > because cube query writing gets all columns queried and checks availability > of those columns in eligible facts. > > The fix is to make the checks happen on each select phrase to be separate. > > Changes include : > - Added QueriedPhraseContext to hold the different queried phrases in all > clauses > - Updated candidate pruning to happen for columns in QueriedPhraseContext > instead of all columns queried > - Removal of a lot of book keeping done withrespect to columns queried or > expressions queried > - Added aggregate as a field in QueriedPhraseContext and updated > GroupbyResolver to make use of the same. > - Also moved alias for select phrase to SelectPhraseContext and updated its > in all relavant places. > > Planning to do some more clean up in a follow up jira wrt > denormalizationResovler and optional dimension tables. > > > Diffs > ----- > > lens-cube/src/main/java/org/apache/lens/cube/parse/AggregateResolver.java > 292868a > lens-cube/src/main/java/org/apache/lens/cube/parse/AliasReplacer.java > 5b48ca4 > lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateFact.java > 01265a5 > > lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTableResolver.java > 83e5088 > lens-cube/src/main/java/org/apache/lens/cube/parse/ColumnResolver.java > 2db5dd1 > lens-cube/src/main/java/org/apache/lens/cube/parse/CubeQueryContext.java > 63ec8b2 > > lens-cube/src/main/java/org/apache/lens/cube/parse/DenormalizationResolver.java > ab1710d > lens-cube/src/main/java/org/apache/lens/cube/parse/ExpressionResolver.java > 5adea6c > lens-cube/src/main/java/org/apache/lens/cube/parse/GroupbyResolver.java > 8beeb9d > lens-cube/src/main/java/org/apache/lens/cube/parse/MultiFactHQLContext.java > 7fbcd7e > > lens-cube/src/main/java/org/apache/lens/cube/parse/QueriedPhraseContext.java > PRE-CREATION > lens-cube/src/main/java/org/apache/lens/cube/parse/SelectPhraseContext.java > PRE-CREATION > lens-cube/src/main/java/org/apache/lens/cube/parse/TimeRangeChecker.java > ca176ee > lens-cube/src/main/java/org/apache/lens/cube/parse/TrackQueriedColumns.java > b65ac26 > > lens-cube/src/main/java/org/apache/lens/cube/parse/TrackQueriedCubeFields.java > PRE-CREATION > > lens-cube/src/main/java/org/apache/lens/cube/parse/TracksQueriedColumns.java > PRE-CREATION > lens-cube/src/test/java/org/apache/lens/cube/parse/CubeTestSetup.java > f7f8af2 > > lens-cube/src/test/java/org/apache/lens/cube/parse/TestAggregateResolver.java > 35234a1 > lens-cube/src/test/java/org/apache/lens/cube/parse/TestBaseCubeQueries.java > 6fb027a > > Diff: https://reviews.apache.org/r/52381/diff/ > > > Testing > ------- > > All cube tests pass. > > > Thanks, > > Amareshwari Sriramadasu > >
