morrySnow commented on code in PR #43546:
URL: https://github.com/apache/doris/pull/43546#discussion_r2168524410
##########
fe/fe-core/src/main/java/org/apache/doris/cluster/ClusterNamespace.java:
##########
@@ -43,8 +43,15 @@ private static boolean checkName(String str) {
if (Strings.isNullOrEmpty(str)) {
return false;
}
- final String[] ele = str.split(CLUSTER_DELIMITER);
- return (ele.length > 1) ? true : false;
+
+ char delimiter = CLUSTER_DELIMITER.charAt(0);
+ int delimiterNum = 0;
+ for (int i = 0; i < str.length(); i++) {
+ if (str.charAt(i) == delimiter) {
+ delimiterNum++;
Review Comment:
nit: could return true here directly
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundRelation.java:
##########
@@ -187,6 +187,28 @@ public String toString() {
return Utils.toSqlString("UnboundRelation", args.toArray());
}
+ @Override
+ public boolean equals(Object o) {
Review Comment:
why need override equals?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/NereidsPlanner.java:
##########
@@ -105,6 +105,7 @@
* Planner to do query plan in Nereids.
*/
public class NereidsPlanner extends Planner {
+ // private static final AtomicInteger executeCount = new AtomicInteger(0);
Review Comment:
what's this? should remove?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SkipSimpleExprs.java:
##########
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.rewrite;
+
+import org.apache.doris.nereids.analyzer.UnboundSlot;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.IsNull;
+import org.apache.doris.nereids.trees.expressions.Slot;
+
+import com.google.common.collect.Maps;
+
+import java.util.Map;
+import java.util.Map.Entry;
+
+/** SkipSimpleExprs */
+public class SkipSimpleExprs {
Review Comment:
add comment to explain the skip detail
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SubqueryToApply.java:
##########
@@ -516,7 +516,6 @@ private Pair<LogicalPlan, Optional<Expression>>
addApply(SubqueryExpr subquery,
new LessThanEqual(countSlot, new
IntegerLiteral(1))),
new VarcharLiteral("correlate scalar subquery must
return only 1 row"))));
logicalProject = new LogicalProject(projects.build(),
newApply);
- logicalProject = new LogicalProject(upperProjects,
logicalProject);
Review Comment:
why not remove variable `upperProjects`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SkipSimpleExprs.java:
##########
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.rewrite;
+
+import org.apache.doris.nereids.analyzer.UnboundSlot;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.IsNull;
+import org.apache.doris.nereids.trees.expressions.Slot;
+
+import com.google.common.collect.Maps;
+
+import java.util.Map;
+import java.util.Map.Entry;
+
+/** SkipSimpleExprs */
+public class SkipSimpleExprs {
+ /** isSimpleExpr */
+ public static boolean isSimpleExpr(Expression expression) {
+ if (expression.containsType(UnboundSlot.class, IsNull.class)) {
+ return false;
+ }
+ ExprFeature exprFeature = computeExprFeature(expression);
+ for (Entry<Integer, Integer> kv : exprFeature.slotCount.entrySet()) {
+ Integer slotId = kv.getKey();
+ Integer count = kv.getValue();
+ if (count > 1) {
+ Integer isNullCount = exprFeature.slotIsNullCount.get(slotId);
+ if (isNullCount == null || isNullCount > 1) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+
+ /** computeExprFeature */
+ public static ExprFeature computeExprFeature(Expression expr) {
+ Map<Integer, Integer> slotCount = Maps.newHashMap();
+ Map<Integer, Integer> slotIsNullCount = Maps.newHashMap();
+ computeExprFeature(expr, slotCount, slotIsNullCount);
+ return new ExprFeature(slotCount, slotIsNullCount);
+ }
+
+ private static void computeExprFeature(
Review Comment:
add comment to explain this function
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PruneOlapScanTablet.java:
##########
@@ -41,24 +45,41 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
-import java.util.Set;
/**
* prune bucket
*/
public class PruneOlapScanTablet extends OneRewriteRuleFactory {
@Override
public Rule build() {
- return logicalFilter(logicalOlapScan()).then(filter -> {
+ return logicalFilter(logicalOlapScan()).thenApply(ctx -> {
+ LogicalFilter<LogicalOlapScan> filter = ctx.root;
+
LogicalOlapScan olapScan = filter.child();
OlapTable table = olapScan.getTable();
Builder<Long> selectedTabletIdsBuilder = ImmutableList.builder();
+
+ Map<String, PartitionColumnFilter> filterMap = new
CaseInsensitiveMap();
+
+ for (Expression conjunct : filter.getConjuncts()) {
+ if (conjunct instanceof EqualTo && conjunct.child(0).isSlot())
{
+ Slot slot = (Slot) conjunct.child(0);
+ if (slot instanceof SlotReference && !((SlotReference)
slot).isVisible()) {
+ continue;
+ }
+ }
Review Comment:
only process slot = literal? not process literal = slot?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundStar.java:
##########
@@ -97,6 +99,36 @@ public UnboundStar(List<String> qualifier,
List<NamedExpression> exceptedSlots,
"replace columns can not be null");
}
+ @Override
+ public Slot toSlot() {
+ return this;
+ }
+
+ @Override
+ public String getName() {
+ return "*";
+ }
+
+ @Override
+ public ExprId getExprId() throws UnboundException {
+ return new ExprId(-1);
+ }
+
+ @Override
+ public Slot withQualifier(List<String> qualifier) {
+ return this;
+ }
+
+ @Override
+ public Slot withName(String name) {
+ return this;
+ }
+
+ @Override
+ public Slot withExprId(ExprId exprId) {
+ return this;
+ }
Review Comment:
after extends Slot these impl is very weird, we should explain it
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundStar.java:
##########
@@ -97,6 +99,36 @@ public UnboundStar(List<String> qualifier,
List<NamedExpression> exceptedSlots,
"replace columns can not be null");
}
+ @Override
+ public Slot toSlot() {
+ return this;
+ }
+
+ @Override
+ public String getName() {
+ return "*";
Review Comment:
should return with qualifier
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Analyzer.java:
##########
@@ -153,15 +156,20 @@ private static List<RewriteJob> buildAnalyzerJobs() {
),
topDown(new LeadingJoin()),
bottomUp(new NormalizeGenerate()),
- bottomUp(new SubqueryToApply())
- /*
- * Notice, MergeProjects rule should NOT be placed after
SubqueryToApply in analyze phase.
- * because in SubqueryToApply, we may add assert_true function with
subquery output slot in projects list.
- * on the other hand, the assert_true function should be not be in
final output.
- * in order to keep the plan unchanged, we add a new project node to
prune the extra assert_true slot.
- * but MergeProjects rule will merge the two projects and keep
assert_true anyway.
- * so we move MergeProjects from analyze to rewrite phase.
- */
+ bottomUp(new SubqueryToApply()),
+ /*
+ * Notice, MergeProjects rule should NOT be placed after
SubqueryToApply in analyze phase.
+ * because in SubqueryToApply, we may add assert_true function
with subquery output slot in projects list.
+ * on the other hand, the assert_true function should be not be in
final output.
+ * in order to keep the plan unchanged, we add a new project node
to prune the extra assert_true slot.
+ * but MergeProjects rule will merge the two projects and keep
assert_true anyway.
+ * so we move MergeProjects from analyze to rewrite phase.
+ */
+ bottomUp(new SubqueryToApply()),
Review Comment:
why do twice SubqueryToApply?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/load/NereidsLoadScanProvider.java:
##########
@@ -317,6 +320,7 @@ private void
fillContextExprMap(List<NereidsImportColumnDesc> columnDescList, Ne
// create scan SlotReferences and transform hadoop functions
boolean hasColumnFromTable = false;
+ IdGenerator<ExprId> exprIdGenerator =
StatementScopeIdGenerator.getExprIdGenerator();
Review Comment:
if u want to avoid `ConnectContext.get()` maybe we could add a test flag
check in all `newXXXId`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -4121,6 +4124,9 @@ protected LogicalPlan withProjection(LogicalPlan input,
SelectColumnClauseContex
throw new ParseException("SELECT * must have a FROM
clause");
}
}
+ if (input instanceof LogicalOneRowRelation) {
+ return new UnboundOneRowRelation(((LogicalOneRowRelation)
input).getRelationId(), projects);
+ }
Review Comment:
why change back to UnboundOneRowRelation, add comment to explain
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/Rules.java:
##########
@@ -0,0 +1,44 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules;
+
+import org.apache.doris.nereids.CascadesContext;
+import org.apache.doris.nereids.trees.TreeNode;
+
+import java.util.List;
+
+/** Rules */
Review Comment:
need a comment to explain what's this and how to use it. every function in
this interface need a comment
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SkipSimpleExprs.java:
##########
@@ -0,0 +1,89 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.rewrite;
+
+import org.apache.doris.nereids.analyzer.UnboundSlot;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.IsNull;
+import org.apache.doris.nereids.trees.expressions.Slot;
+
+import com.google.common.collect.Maps;
+
+import java.util.Map;
+import java.util.Map.Entry;
+
+/** SkipSimpleExprs */
+public class SkipSimpleExprs {
+ /** isSimpleExpr */
+ public static boolean isSimpleExpr(Expression expression) {
+ if (expression.containsType(UnboundSlot.class, IsNull.class)) {
+ return false;
+ }
+ ExprFeature exprFeature = computeExprFeature(expression);
+ for (Entry<Integer, Integer> kv : exprFeature.slotCount.entrySet()) {
+ Integer slotId = kv.getKey();
+ Integer count = kv.getValue();
+ if (count > 1) {
+ Integer isNullCount = exprFeature.slotIsNullCount.get(slotId);
+ if (isNullCount == null || isNullCount > 1) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+
+ /** computeExprFeature */
+ public static ExprFeature computeExprFeature(Expression expr) {
+ Map<Integer, Integer> slotCount = Maps.newHashMap();
+ Map<Integer, Integer> slotIsNullCount = Maps.newHashMap();
+ computeExprFeature(expr, slotCount, slotIsNullCount);
+ return new ExprFeature(slotCount, slotIsNullCount);
+ }
+
+ private static void computeExprFeature(
+ Expression e, Map<Integer, Integer> slotCount, Map<Integer,
Integer> slotIsNullCount) {
+ if (e instanceof Slot) {
+ if (!(e instanceof UnboundSlot)) {
+ int slotId = ((Slot) e).getExprId().asInt();
+ Integer count = slotCount.get(slotId);
+ slotCount.put(slotId, count == null ? 1 : count + 1);
+ }
+ } else if (e instanceof IsNull && e.child(0) instanceof Slot) {
+ if (!(e.child(0) instanceof UnboundSlot)) {
+ int slotId = ((Slot) e.child(0)).getExprId().asInt();
+ Integer count = slotIsNullCount.get(slotId);
+ slotIsNullCount.put(slotId, count == null ? 1 : count + 1);
+ }
+ } else {
+ for (Expression child : e.children()) {
+ computeExprFeature(child, slotCount, slotIsNullCount);
+ }
+ }
+ }
+
+ private static class ExprFeature {
Review Comment:
add comment to explain the two attributes
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/SuperClassId.java:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees;
+
+import java.util.BitSet;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/** SuperClassId */
+// NOTE: static method let jvm do more aggressive inline, so we not make the
instance
+public class SuperClassId {
Review Comment:
add comment to explain where and how to use it
--
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]
