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. 


---

Reply via email to