----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20741/#review41681 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToMergeExchange.java <https://reviews.apache.org/r/20741/#comment75239> This looks like old, unused code. Do we need this? exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillFilterRelBase.java <https://reviews.apache.org/r/20741/#comment75240> I think we should probably add a cost settings/config object. Could hang off PlannerSettings or used to initialize our RelOptCostFactory. I'd like to avoid magic numbers being spread throughout the code so we can change them in a local place and evaluate the impacts exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillProjectRelBase.java <https://reviews.apache.org/r/20741/#comment75241> Let's try to include this magic number also in the centralized cost config so we can explore its impact. As above, ideally we would pull this from an understanding of the expressions. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/DrillDistributionTraitDef.java <https://reviews.apache.org/r/20741/#comment75243> How does OrderedPartitionExchange destroy orderedness? Same for broadcast? exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrel.java <https://reviews.apache.org/r/20741/#comment75245> Ideally, this shouldn't be hit. Rule should fail to match. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrule.java <https://reviews.apache.org/r/20741/#comment75244> Can you add a matches which checks the aggregates and fails to match on the case where there is a distinct aggregate? exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrule.java <https://reviews.apache.org/r/20741/#comment75246> probably add note here about multi phase aggregate exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashAggPrule.java <https://reviews.apache.org/r/20741/#comment75247> why did you disable single grouping key? exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java <https://reviews.apache.org/r/20741/#comment75248> As above, let's primarily do this check in the rule rather than waiting for exceptions. Exceptions are expensive. Additionally, we have warning messages that are misleading because an alternative plan may avoid an equijoin. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashJoinPrel.java <https://reviews.apache.org/r/20741/#comment75249> please centralize magic number. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToMergeExchangePrel.java <https://reviews.apache.org/r/20741/#comment75250> Can we update to new utility method? Also, are we sure this is true. I thought hash exchange supported all selection vectors. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/MergeJoinPrule.java <https://reviews.apache.org/r/20741/#comment75251> This brings to mind that we should probably have settings to enable disable various types of exchanges. E.g. supports_broadcast_exchange exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/ScanPrel.java <https://reviews.apache.org/r/20741/#comment75252> I could be wrong but I thought the third number was disk io. Why would this be 0? Also, we should probably delegate this down to groupscan as different scan types have different costs. For example, Parquet is substantially more CPU intensive. CSV, more disk intensive. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamAggPrule.java <https://reviews.apache.org/r/20741/#comment75253> same as hash agg, why removed? exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamAggPrule.java <https://reviews.apache.org/r/20741/#comment75254> maybe note that this is incomplete and thus commented out. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/StreamAggPrule.java <https://reviews.apache.org/r/20741/#comment75255> I think this is the same as StreamAgg, can we move to shared location? - Jacques Nadeau 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 > >
