phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From JamesRTaylor <...@git.apache.org>
Subject [GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
Date Fri, 17 Nov 2017 08:11:37 GMT
Github user JamesRTaylor commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/281#discussion_r151623809
  
    --- Diff: phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java ---
    @@ -91,8 +91,23 @@ public QueryPlan optimize(PhoenixStatement statement, SelectStatement
select, Co
         }
         
         public QueryPlan optimize(QueryPlan dataPlan, PhoenixStatement statement, List<?
extends PDatum> targetColumns, ParallelIteratorFactory parallelIteratorFactory) throws
SQLException {
    -        List<QueryPlan>plans = getApplicablePlans(dataPlan, statement, targetColumns,
parallelIteratorFactory, true);
    -        return plans.get(0);
    +        List<QueryPlan> plans = getApplicablePlans(dataPlan, statement, targetColumns,
parallelIteratorFactory, false);
    +        if (plans.size() == 1) {
    +            return plans.get(0);
    +        }
    +
    +        // Get the best plan based on their costs. Costs will be ZERO if stats are not
    +        // available, thus the first plan will be returned.
    +        Cost minCost = null;
    +        QueryPlan bestPlan = null;
    +        for (QueryPlan plan : plans) {
    +            Cost cost = plan.getCost();
    +            if (minCost == null || cost.compareTo(minCost) < 0) {
    +                minCost = cost;
    +                bestPlan = plan;
    +            }
    +        }
    +        return bestPlan;
    --- End diff --
    
    I agree with @katameru. If you plug in your optimization code to impact the sort order
done by QueryOptimizer.orderPlansBestToWorst() when getApplicablePlans() is called, we'd keep
the behavior that the hints would override the optimizer (whether cost-based is enabled or
not).
    
    I still think we need more tests. We've found that if the tests don't get added with the
initial commit, they unfortunately never get added. 


---

Mime
View raw message