morrySnow commented on code in PR #11604:
URL: https://github.com/apache/doris/pull/11604#discussion_r941116524
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/cascades/CostAndEnforcerJob.java:
##########
@@ -66,18 +67,18 @@ public CostAndEnforcerJob(GroupExpression groupExpression,
JobContext context) {
this.groupExpression = groupExpression;
}
- @Override
- public void execute() {
- for (Group childGroup : groupExpression.children()) {
- if (!childGroup.isHasCost()) {
- // TODO: interim solution
- pushTask(new CostAndEnforcerJob(this.groupExpression,
context));
- pushTask(new OptimizeGroupJob(childGroup, context));
- childGroup.setHasCost(true);
- return;
- }
- }
- }
+ // @Override
+ // public void execute1() {
Review Comment:
remove it directly?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/cost/CostCalculator.java:
##########
@@ -43,10 +43,11 @@ public class CostCalculator {
* Constructor.
*/
public static double calculateCost(GroupExpression groupExpression) {
- PlanContext planContext = new PlanContext(groupExpression);
- CostEstimator costCalculator = new CostEstimator();
- CostEstimate costEstimate =
groupExpression.getPlan().accept(costCalculator, planContext);
- return costFormula(costEstimate);
+ // PlanContext planContext = new PlanContext(groupExpression);
Review Comment:
add a comment to explain why comment these code.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -74,20 +74,13 @@ public PhysicalProperties
visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> has
PhysicalProperties rightOutputProperty =
childrenOutputProperties.get(1);
// broadcast
+ // TODO
Review Comment:
TODO what?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -112,6 +113,7 @@ public static Pair<List<SlotReference>,
List<SlotReference>> getOnClauseUsedSlot
}
}
+ Preconditions.checkArgument(childSlots.first.size() ==
childSlots.second.size());
Review Comment:
```suggestion
Preconditions.checkState(childSlots.first.size() ==
childSlots.second.size());
```
https://guava.dev/releases/19.0/api/docs/com/google/common/base/Preconditions.html
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/RequestPropertyDeriver.java:
##########
@@ -71,21 +71,21 @@ public Void visit(Plan plan, PlanContext context) {
@Override
public Void visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> hashJoin,
PlanContext context) {
- // for broadcast join
- List<PhysicalProperties> propertiesForBroadcast = Lists.newArrayList(
- new PhysicalProperties(),
- new PhysicalProperties(new DistributionSpecReplicated())
- );
// for shuffle join
- Pair<List<SlotReference>, List<SlotReference>> onClauseUsedSlots =
JoinUtils.getOnClauseUsedSlots(hashJoin);
- List<PhysicalProperties> propertiesForShuffle = Lists.newArrayList(
- new PhysicalProperties(new
DistributionSpecHash(onClauseUsedSlots.first, ShuffleType.JOIN)),
- new PhysicalProperties(new
DistributionSpecHash(onClauseUsedSlots.second, ShuffleType.JOIN)));
-
if (!JoinUtils.onlyBroadcast(hashJoin)) {
+ Pair<List<SlotReference>, List<SlotReference>> onClauseUsedSlots =
JoinUtils.getOnClauseUsedSlots(hashJoin);
+ List<PhysicalProperties> propertiesForShuffle = Lists.newArrayList(
+ new PhysicalProperties(new
DistributionSpecHash(onClauseUsedSlots.first, ShuffleType.JOIN)),
+ new PhysicalProperties(new
DistributionSpecHash(onClauseUsedSlots.second, ShuffleType.JOIN)));
+
requestPropertyToChildren.add(propertiesForShuffle);
}
+ // for broadcast join
if (!JoinUtils.onlyShuffle(hashJoin)) {
+ List<PhysicalProperties> propertiesForBroadcast =
Lists.newArrayList(
+ new PhysicalProperties(),
Review Comment:
i think it is better to create any distribution properties explicitly. such
as
```
new PhysicalProperties(DistributionSpecAny.getInstance())
```
or
```
PhysicalProperties.CreateDistributionSpecAnyInstance();
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -74,20 +74,13 @@ public PhysicalProperties
visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> has
PhysicalProperties rightOutputProperty =
childrenOutputProperties.get(1);
// broadcast
+ // TODO
if (rightOutputProperty.getDistributionSpec() instanceof
DistributionSpecReplicated) {
- // TODO
return leftOutputProperty;
}
// shuffle
- // List<SlotReference> leftSlotRefs =
hashJoin.left().getOutput().stream().map(slot -> (SlotReference) slot)
- // .collect(Collectors.toList());
- // List<SlotReference> rightSlotRefs =
hashJoin.right().getOutput().stream().map(slot -> (SlotReference) slot)
- // .collect(Collectors.toList());
-
- // List<SlotReference> leftOnSlotRefs;
- // List<SlotReference> rightOnSlotRefs;
- // Preconditions.checkState(leftOnSlotRefs.size() ==
rightOnSlotRefs.size());
+ // TODO
Review Comment:
ditto
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalOlapScanToPhysicalOlapScan.java:
##########
@@ -31,12 +44,35 @@ public class LogicalOlapScanToPhysicalOlapScan extends
OneImplementationRuleFact
@Override
public Rule build() {
return logicalOlapScan().then(olapScan ->
- // TODO: olapScan should get (OlapTable);
- new PhysicalOlapScan(
- (OlapTable) olapScan.getTable(),
- olapScan.getQualifier(),
- Optional.empty(),
- olapScan.getLogicalProperties())
+ new PhysicalOlapScan(
+ (OlapTable) olapScan.getTable(),
+ olapScan.getQualifier(),
+ convertDistribution(olapScan),
+ Optional.empty(),
+ olapScan.getLogicalProperties())
).toRule(RuleType.LOGICAL_OLAP_SCAN_TO_PHYSICAL_OLAP_SCAN_RULE);
}
+
+ private DistributionSpec convertDistribution(LogicalOlapScan olapScan) {
+ DistributionInfo distributionInfo =
olapScan.getTable().getDefaultDistributionInfo();
+ if (distributionInfo instanceof HashDistributionInfo) {
+ HashDistributionInfo hashDistributionInfo = (HashDistributionInfo)
distributionInfo;
+
+ List<SlotReference> output =
olapScan.getOutput().stream().map(slot -> (SlotReference) slot)
Review Comment:
```suggestion
List<SlotReference> output =
olapScan.getOutput().stream().map(SlotReference.class:cast)
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -78,7 +78,8 @@ private static boolean isEqualTo(List<SlotReference>
leftSlots, List<SlotReferen
return false;
}
- return Utils.equalsIgnoreOrder(leftUsed, leftSlots) ||
Utils.equalsIgnoreOrder(rightUsed, rightSlots);
+ return (new HashSet<>(leftSlots).containsAll(leftUsed) && new
HashSet<>(rightSlots).containsAll(rightUsed))
+ || (new HashSet<>(leftSlots).containsAll(rightUsed) && new
HashSet<>(rightSlots).containsAll(leftUsed));
Review Comment:
```suggestion
Set<> leftSlotsSet = new HashSet<>(leftSlots);
Set<> rightSlotsSet = new HashSet<>(rightSlots);
...
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalOlapScanToPhysicalOlapScan.java:
##########
@@ -31,12 +44,35 @@ public class LogicalOlapScanToPhysicalOlapScan extends
OneImplementationRuleFact
@Override
public Rule build() {
return logicalOlapScan().then(olapScan ->
- // TODO: olapScan should get (OlapTable);
- new PhysicalOlapScan(
- (OlapTable) olapScan.getTable(),
- olapScan.getQualifier(),
- Optional.empty(),
- olapScan.getLogicalProperties())
+ new PhysicalOlapScan(
+ (OlapTable) olapScan.getTable(),
Review Comment:
add a Override function in LogicalOlapScan, and then remove cast on
`olapScan.getTable()`
```java
@Override
OlapTable getTable();
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/DistributionSpecHash.java:
##########
@@ -68,7 +68,7 @@ public boolean satisfy(DistributionSpec other) {
// TODO: need consider following logic whether is right, and maybe
need consider more.
// Current shuffleType is LOCAL/AGG, allow if current is contained by
other
- if (shuffleType == ShuffleType.LOCAL && spec.shuffleType ==
ShuffleType.AGG) {
+ if (shuffleType == ShuffleType.LOCAL || spec.shuffleType ==
ShuffleType.AGG) {
Review Comment:
better to add an example to explain why agg is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
