phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Samarth Jain (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (PHOENIX-2377) Use a priority queue in MergeSortResultIterator
Date Tue, 10 Nov 2015 00:01:10 GMT

    [ https://issues.apache.org/jira/browse/PHOENIX-2377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14997692#comment-14997692
] 

Samarth Jain commented on PHOENIX-2377:
---------------------------------------

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)

Mime
View raw message