You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Lavkesh Lahngir <la...@gmail.com> on 2017/02/20 17:39:58 UTC
Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/
-----------------------------------------------------------
Review request for lens.
Bugs: LENS-1391
https://issues.apache.org/jira/browse/LENS-1391
Repository: lens
Description
-------
MaxCoveringFactResolver and LeastPartitionResolver review comments
Diffs
-----
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047b57ebc26244891d9bc48ad18ab0176fdd
lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df245cd2b79ee29991f98df36f57e4e029d2f
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde1786c2e6217a0d7aa7a26d1cc248bf532
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec88b1dd0507d21ca1b414dcc6fb68317e3
Diff: https://reviews.apache.org/r/56858/diff/
Testing
-------
Thanks,
Lavkesh Lahngir
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Lavkesh Lahngir <la...@gmail.com>.
> On Feb. 21, 2017, 7:11 a.m., Puneet Gupta wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java, line 156
> > <https://reviews.apache.org/r/56858/diff/1/?file=1640583#file1640583line156>
> >
> > Removed ?
This is being calculated unnecessary.
- Lavkesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166126
-----------------------------------------------------------
On Feb. 21, 2017, 10:10 a.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 10:10 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Puneet Gupta <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166126
-----------------------------------------------------------
1. Can you commnet on https://github.com/sushilmohanty/incubator-lens/pull/4 and metion which all comments are resolved
2. Can you run test cases for lens-cube
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java (line 257)
<https://reviews.apache.org/r/56858/#comment238024>
Comment is redundant ?
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java
<https://reviews.apache.org/r/56858/#comment238027>
Removed ?
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java
<https://reviews.apache.org/r/56858/#comment238028>
Removed ?
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java
<https://reviews.apache.org/r/56858/#comment238029>
Removed ?
- Puneet Gupta
On Feb. 20, 2017, 5:39 p.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2017, 5:39 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047b57ebc26244891d9bc48ad18ab0176fdd
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df245cd2b79ee29991f98df36f57e4e029d2f
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde1786c2e6217a0d7aa7a26d1cc248bf532
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec88b1dd0507d21ca1b414dcc6fb68317e3
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Lavkesh Lahngir <la...@gmail.com>.
> On Feb. 21, 2017, 11:19 a.m., Puneet Gupta wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java, line 70
> > <https://reviews.apache.org/r/56858/diff/2/?file=1640673#file1640673line70>
> >
> > This should be done at candidate level and not at storage candidate level.
> > Candidate.getParticipatingPartitions().size()
Yeah. makes sense.. And the total size is equal to the addition of all the child parition sizes.
> On Feb. 21, 2017, 11:19 a.m., Puneet Gupta wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java, line 125
> > <https://reviews.apache.org/r/56858/diff/2/?file=1640674#file1640674line125>
> >
> > passing null ?
> > Should pass the incomplete partition details . This info will be stored in CandidateTablePruneCause#incompletePartitions and will be used for error message construction.
hmm. This is at storageCandidate level not at candidate level.
> On Feb. 21, 2017, 11:19 a.m., Puneet Gupta wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java, line 544
> > <https://reviews.apache.org/r/56858/diff/2/?file=1640675#file1640675line544>
> >
> > Why this comment is dropped ?
Yeah.. I will put that again.
> On Feb. 21, 2017, 11:19 a.m., Puneet Gupta wrote:
> > lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java, lines 72-75
> > <https://reviews.apache.org/r/56858/diff/2/?file=1640675#file1640675line72>
> >
> > Older comments seem more readable. Can you check once.
Yes.. I will undo it.
- Lavkesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166148
-----------------------------------------------------------
On Feb. 21, 2017, 11:19 a.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 11:19 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Puneet Gupta <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166148
-----------------------------------------------------------
lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java (line 70)
<https://reviews.apache.org/r/56858/#comment238032>
This should be done at candidate level and not at storage candidate level.
Candidate.getParticipatingPartitions().size()
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java (line 122)
<https://reviews.apache.org/r/56858/#comment238033>
passing null ?
Should pass the incomplete partition details . This info will be stored in CandidateTablePruneCause#incompletePartitions and will be used for error message construction.
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java
<https://reviews.apache.org/r/56858/#comment238034>
Older comments seem more readable. Can you check once.
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java
<https://reviews.apache.org/r/56858/#comment238035>
Why this comment is dropped ?
- Puneet Gupta
On Feb. 21, 2017, 11:19 a.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 11:19 a.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Puneet Gupta <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166635
-----------------------------------------------------------
Ship it!
- Puneet Gupta
On Feb. 21, 2017, 12:12 p.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 12:12 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Puneet Gupta <pu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/#review166167
-----------------------------------------------------------
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java (line 122)
<https://reviews.apache.org/r/56858/#comment238052>
But null does not make sense. We should pass the incomplete partitions. Lets have an online discussion on this if you feel otherwise.
cubeql.addCandidatePruningMsg() actually adds the message at StorageCandidate Level .
- Puneet Gupta
On Feb. 21, 2017, 12:12 p.m., Lavkesh Lahngir wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56858/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 12:12 p.m.)
>
>
> Review request for lens.
>
>
> Bugs: LENS-1391
> https://issues.apache.org/jira/browse/LENS-1391
>
>
> Repository: lens
>
>
> Description
> -------
>
> MaxCoveringFactResolver and LeastPartitionResolver review comments
>
>
> Diffs
> -----
>
> lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
> lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
> lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
> lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
>
> Diff: https://reviews.apache.org/r/56858/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lavkesh Lahngir
>
>
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Lavkesh Lahngir <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/
-----------------------------------------------------------
(Updated Feb. 21, 2017, 12:12 p.m.)
Review request for lens.
Bugs: LENS-1391
https://issues.apache.org/jira/browse/LENS-1391
Repository: lens
Description
-------
MaxCoveringFactResolver and LeastPartitionResolver review comments
Diffs (updated)
-----
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
Diff: https://reviews.apache.org/r/56858/diff/
Testing
-------
Thanks,
Lavkesh Lahngir
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Lavkesh Lahngir <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/
-----------------------------------------------------------
(Updated Feb. 21, 2017, 11:19 a.m.)
Review request for lens.
Bugs: LENS-1391
https://issues.apache.org/jira/browse/LENS-1391
Repository: lens
Description
-------
MaxCoveringFactResolver and LeastPartitionResolver review comments
Diffs (updated)
-----
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
Diff: https://reviews.apache.org/r/56858/diff/
Testing
-------
Thanks,
Lavkesh Lahngir
Re: Review Request 56858: Review comment fixes for
MaxCoveringFactResolver and LeastPartitionResolver
Posted by Lavkesh Lahngir <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56858/
-----------------------------------------------------------
(Updated Feb. 21, 2017, 10:10 a.m.)
Review request for lens.
Bugs: LENS-1391
https://issues.apache.org/jira/browse/LENS-1391
Repository: lens
Description
-------
MaxCoveringFactResolver and LeastPartitionResolver review comments
Diffs (updated)
-----
lens-cube/src/main/java/org/apache/lens/cube/parse/CandidateTablePruneCause.java c7f2047
lens-cube/src/main/java/org/apache/lens/cube/parse/LeastPartitionResolver.java 153df24
lens-cube/src/main/java/org/apache/lens/cube/parse/MaxCoveringFactResolver.java 4664cde
lens-cube/src/main/java/org/apache/lens/cube/parse/StorageCandidate.java fca86ec
Diff: https://reviews.apache.org/r/56858/diff/
Testing
-------
Thanks,
Lavkesh Lahngir