You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Amareshwari Sriramadasu <am...@apache.org> on 2016/09/29 09:49:29 UTC
Review Request 52381: LENS-1273 : Resolve issues with case when
aggregate expressions with dim-attributes conditions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52381/
-----------------------------------------------------------
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
Re: Review Request 52381: LENS-1273 : Resolve issues with case when
aggregate expressions with dim-attributes conditions
Posted by Amareshwari Sriramadasu <am...@apache.org>.
> On Oct. 6, 2016, 10:52 a.m., Rajat Khandelwal wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/ColumnResolver.java, line 222
> > <https://reviews.apache.org/r/52381/diff/1/?file=1514926#file1514926line222>
> >
> > isn't `alias!=null` always true inside this block?
alias is set only when user passed alias
> On Oct. 6, 2016, 10:52 a.m., Rajat Khandelwal wrote:
> > lens-cube/src/test/java/org/apache/lens/cube/parse/TestBaseCubeQueries.java, line 744
> > <https://reviews.apache.org/r/52381/diff/1/?file=1514940#file1514940line744>
> >
> > Is join happening on dim22 also?
no. It wont.
- Amareshwari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52381/#review151478
-----------------------------------------------------------
On Sept. 29, 2016, 9:49 a.m., Amareshwari Sriramadasu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52381/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2016, 9:49 a.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
>
>
Re: Review Request 52381: LENS-1273 : Resolve issues with case when
aggregate expressions with dim-attributes conditions
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 52381: LENS-1273 : Resolve issues with case when
aggregate expressions with dim-attributes conditions
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52381/
-----------------------------------------------------------
(Updated Oct. 7, 2016, 8:42 a.m.)
Review request for lens and Rajat Khandelwal.
Changes
-------
Review comments fixed
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 (updated)
-----
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 b3547db
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 053cad3
Diff: https://reviews.apache.org/r/52381/diff/
Testing
-------
All cube tests pass.
Thanks,
Amareshwari Sriramadasu