[
https://issues.apache.org/jira/browse/PHOENIX-2377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14997692#comment-14997692
]
Samarth Jain edited comment on PHOENIX-2377 at 11/10/15 12:01 AM:
------------------------------------------------------------------
Patch looks pretty good [~ankit.singhal]. Almost there! I have a few
suggestions:
1) In MaterializedComparableResultIterator, I would get rid of the member
variable firstElement and the method getFirstElement() since you already have
that information available when the peek() is called for the first time on the
iterator. I would also change the compareTo method to :
{code}
+ @Override
+ public int compareTo(MaterializedComparableResultIterator o){
+ return comparator.compare(this.peek(), o.peek());
+
+ }
{code}
2) In MaterializedComparableResultIterator rename the iterator member variable
to delegate. Also, add the implementation for getExplainPlan() to call
delegate.explain().
3) Minor nit: remove + * @since 0.1 from the class level comment in
MaterializedComparableResultIterator
4) In MergeSortResultIterator, the createMinHeap() method should be renamed to
getMinHeap() (because you are only creating the heap once). Also, IMHO, the
loop reads better by not using the explicit i index. You can also get away with
creating a single final private instance for IteratorComparator() instead of
creating a new one for every iterator being added to the heap. It should be
safe since there isn't any state stored in the IteratorComparator. It would
also be better to close the underlying iterator if the peek returns no results
(to free up resources sooner). Something like this :
{code}
+ private final IteratorComparator COMPARATOR = new IteratorComparator();
+
+
+ private PriorityQueue<MaterializedComparableResultIterator> getMinHeap()
throws SQLException{
+ if(minHeap == null){
+ List<PeekingResultIterator> iterators = getIterators();
+ minHeap = new
PriorityQueue<MaterializedComparableResultIterator>(iterators.size());
+ for (PeekingResultIterator itr : iterators) {
+ if (itr.peek()==null) {
+ itr.close(); // free up resources sooner
+ }
+ minHeap.add(new MaterializedComparableResultIterator(itr,
COMPARATOR));
+ }
+ }
+ return minHeap;
+ }
{code}
5) Make sure that the code guidelines are adhered to. For more details on this
see http://phoenix.apache.org/develop.html and the Get Settings and Preferences
Correct section in particular.
was (Author: samarthjain):
Patch looks pretty good [~ankit.singhal]. Almost there! I have a few
suggestions:
1) In MaterializedComparableResultIterator, I would get rid of the member
variable firstElement and the method getFirstElement() since you already have
that information available when the peek() is called for the first time on the
iterator. I would also change the compareTo method to :
{code}
+ @Override
+ public int compareTo(MaterializedComparableResultIterator o){
+ return comparator.compare(this.peek(), o.peek());
+
+ }
{code}
2) In MaterializedComparableResultIterator rename the iterator member variable
to delegate. Also, add the implementation for getExplainPlan() to call
delegate.explain().
3) Minor nit: remove + * @since 0.1 from the class level comment in
MaterializedComparableResultIterator
4) In MergeSortResultIterator, the createMinHeap() method should be renamed to
getMinHeap() (because you are only creating the heap once). Also, IMHO, the
loop reads better by not using the explicit i index. You can also get away with
creating a single final private instance for IteratorComparator() instead of
creating a new one for every iterator being added to the heap. It should be
safe since there isn't any state stored in the IteratorComparator. It would
also be better to close the underlying iterator if the peek returns no results
(to free up resources sooner). Something like this :
{code}
+ private final IteratorComparator COMPARATOR = new IteratorComparator();
+
+
+ private PriorityQueue<MaterializedComparableResultIterator> getMinHeap()
throws SQLException{
+ if(minHeap == null){
+ List<PeekingResultIterator> iterators = getIterators();
+ minHeap = new
PriorityQueue<MaterializedComparableResultIterator>(iterators.size());
+ for (PeekingResultIterator itr : iterators) {
+ if (itr.peek()==null) {
+ itr.close(); // free up resources sooner
+ }
+ minHeap.add(new MaterializedComparableResultIterator(itr,
COMPARATOR));
+ }
+ }
+ return minHeap;
+ }
5) Make sure that the code guidelines are adhered to. For more details on this
see http://phoenix.apache.org/develop.html and the Get Settings and Preferences
Correct section in particular.
> Use a priority queue in MergeSortResultIterator
> -----------------------------------------------
>
> Key: PHOENIX-2377
> URL: https://issues.apache.org/jira/browse/PHOENIX-2377
> Project: Phoenix
> Issue Type: Sub-task
> Reporter: Ankit Singhal
> Priority: Minor
> Attachments: PHOENIX-2377_v1.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
