[jira] [Comment Edited] (PHOENIX-2377) Use a priority queue in MergeSortResultIterator

["Samarth Jain (JIRA)" <ji...@apache.org>]

    [ 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)