morrySnow commented on code in PR #15240:
URL: https://github.com/apache/doris/pull/15240#discussion_r1059777460
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java:
##########
@@ -45,11 +48,31 @@ public class CheckAfterRewrite extends
OneAnalysisRuleFactory {
public Rule build() {
return any().then(plan -> {
checkAllSlotReferenceFromChildren(plan);
+ checkNoUnbounded(plan);
Review Comment:
we have check bound in
org.apache.doris.nereids.rules.analysis.CheckAnalysis#checkBound.
But it only check unbound slot.
If u want to check UnboundFunction, update it~
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java:
##########
@@ -45,11 +48,31 @@ public class CheckAfterRewrite extends
OneAnalysisRuleFactory {
public Rule build() {
return any().then(plan -> {
checkAllSlotReferenceFromChildren(plan);
+ checkNoUnbounded(plan);
checkMetricTypeIsUsedCorrectly(plan);
return null;
}).toRule(RuleType.CHECK_ANALYSIS);
}
+ private void checkNoUnbounded(Plan plan) {
+ List<Expression> unboundedExprs = Lists.newArrayList();
+ boolean unbounded = plan.getExpressions().stream().anyMatch(
+ expr -> {
+ if
(expr.getInputSlots().stream().anyMatch(UnboundSlot.class::isInstance)) {
+ unboundedExprs.add(expr);
+ return true;
+ } else if
(expr.getInputSlots().stream().anyMatch(UnboundFunction.class::isInstance)) {
+ unboundedExprs.add(expr);
+ return true;
+ }
+ return false;
Review Comment:
both `UnboundSlot` and `UnboundFunction` are implement `Unbound`.
just need to match `Unbound`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSlotReference.java:
##########
@@ -194,11 +195,55 @@ public List<Rule> buildRules() {
// The columns referenced in group by are first obtained
from the child's output,
// and then from the node's output
+ Set<String> duplicatedSlotNames = new HashSet<>();
Map<String, Expression> childOutputsToExpr =
agg.child().getOutput().stream()
- .collect(Collectors.toMap(Slot::getName,
Slot::toSlot, (oldExpr, newExpr) -> oldExpr));
+ .collect(Collectors.toMap(Slot::getName,
Slot::toSlot,
+ (oldExpr, newExpr) -> {
+ duplicatedSlotNames.add(((Slot)
oldExpr).getName());
+ return oldExpr;
+ }));
Review Comment:
i don't think the original code is right. and i also don't think remove
duplicatedSlotNames output is right.
u could try
```sql
-- t1: a, b, c
-- t2: a, b, c
SELECT SUM(t1.c) FROM t1 JOIN t2 GROUP BY t1.a;
```
the map is not necessary.
the right way is binding twice.
first, bind with children's output. if expr still has unboundSlot, we bind
the **ORIGINAL** unbound expr with aggregate's output. if expr still has
unboundSlot, just throw AnalysisException.
But, i know why we cannot do it now. the BIND_SLOT rule and BIND_FUNCTION
rule is seperated and both throw unbound exception immediately. So we need to
refactor all bind rules **LATER**. **Do not need to fix it in this PR**
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
