> On April 27, 2014, 11:28 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillFilterRelBase.java, > > line 58 > > <https://reviews.apache.org/r/20741/diff/1/?file=569043#file569043line58> > > > > How do we get the number 2 in this formula for Filter op? > > > >
That factor was chosen just to make the cost higher than the base cpu cost but I agree it can be modeled better...I can make it DrillCostBase.compareCpuCost since evaluating a filter would cost at least as much as comparison. Since filters can be arbitrarily complex, one would ideally want to look at the cost of each component differently...for example 'a1 = 1 AND b1 IN (10, 20)' the right thing to do would be to cost the IN predicate differently than the a1=1 predicate. However, this could be future enhancement. For now, I can make the formula as follows: cpuCost = DrillCostBase.compareCpuCost * inputRows * num_conditions where num_conditions = number of conditions in the filter. Let me know if this is reasonable. > On April 27, 2014, 11:28 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillProjectRelBase.java, > > line 63 > > <https://reviews.apache.org/r/20741/diff/1/?file=569044#file569044line63> > > > > Why do we multiple by 0.1 here? I kept the 0.1 factor that was there before. I think Project costing should be adjusted anyways to model the Project push-down feature. Let's talk about this. > On April 27, 2014, 11:28 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java, > > line 41 > > <https://reviews.apache.org/r/20741/diff/1/?file=569047#file569047line41> > > > > In Drill, I think static final should use convention of ALL_UPPERCASE. ok, will change to use uppercase. > On April 27, 2014, 11:28 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java, > > line 182 > > <https://reviews.apache.org/r/20741/diff/1/?file=569047#file569047line182> > > > > Here, why do we add rowCount together with CPU, IO, NETWORK? Are they > > in the same scale (having the same unit)? > > > > I thought cpu, io, network will all computed from rowCount. But when we > > compare two costs, rowCount would not play directly. They have the same data type (double). I suppose logically it makes sense to compare row counts with another and combine the rest in one group. I can do that. > On April 27, 2014, 11:28 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrel.java, > > line 1 > > <https://reviews.apache.org/r/20741/diff/1/?file=569055#file569055line1> > > > > Do you think it would be good to separate the cost model change from > > the new optimizer plan support for HashAgg/HashJoin, and put them into two > > different JIRA? Probably. I could split those out if other reviewers also feel the same. - Aman ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20741/#review41572 ----------------------------------------------------------- On April 26, 2014, 1:54 a.m., Aman Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20741/ > ----------------------------------------------------------- > > (Updated April 26, 2014, 1:54 a.m.) > > > Review request for drill, Jacques Nadeau and Jinfeng Ni. > > > Repository: drill-git > > > Description > ------- > > 1. Added DrillCostBase that includes distribution (network) cost. > 2. Added cost formulas for computeSelfCost() for various Exchange operators, > joins, aggregations etc. > 3. Added Prels for HashJoin, HashAggregate and generate plans for those. > 4. Added exchange Prels: BroadcastExchangePrel and a new type called > HashToMergeExchangePrel. > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java > 7e3b63d > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToMergeExchange.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableConfig.java > 98892c0 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillFilterRelBase.java > 955729b > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillProjectRelBase.java > cf3d188 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillScanRelBase.java > b370352 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillScreenRelBase.java > 51ed442 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillCostBase.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillRelOptCost.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillRelOptCostFactory.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java > 1492a28 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/BroadcastExchangePrel.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/DrillDistributionTrait.java > b75fb40 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/DrillDistributionTraitDef.java > c2ebb7a > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/FilterPrel.java > 0fc3abd > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrel.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrule.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrule.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToMergeExchangePrel.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToRandomExchangePrel.java > e5c9661 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/MergeJoinPrel.java > 978a531 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/MergeJoinPrule.java > 8298e50 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java > e6e99c0 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/ScanPrel.java > a945129 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/SingleMergeExchangePrel.java > d9431cc > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/SortPrel.java > 344be4e > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamAggPrel.java > c2880da > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamAggPrule.java > a561a61 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/TopNPrel.java > e981a45 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/UnionExchangePrel.java > f89cbaa > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java > 8892a8f > exec/java-exec/src/test/java/org/apache/drill/TestTpchSingleMode.java > 1ccb65c > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestHashJoin.java > ba067e2 > exec/java-exec/src/test/resources/join/hj_exchanges.json PRE-CREATION > exec/java-exec/src/test/resources/queries/tpch/01.sql f33416f > > Diff: https://reviews.apache.org/r/20741/diff/ > > > Testing > ------- > > Existing unit tests. Some TPCH tests show memory leaks but that is not > directly related to the costing changes...however those leaks would have to > be resolved before enabling the new plans. However, I wanted to get this > review request out to get feedback. > > > Thanks, > > Aman Sinha > >
