suibianwanwank commented on code in PR #4392: URL: https://github.com/apache/calcite/pull/4392#discussion_r2154175490
########## core/src/main/java/org/apache/calcite/rex/RexNodeAndFieldIndex.java: ########## @@ -0,0 +1,99 @@ +/* + * 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.calcite.rex; + +import org.apache.calcite.rel.type.RelDataType; + +import org.checkerframework.checker.nullness.qual.Nullable; + +import java.util.Objects; + +/** + * RexNodeAndFieldIndex has the same meaning as {@link RexInputRef}, they are both reference a + * field of an input relational expression. The difference is that, RexNodeAndFieldIndex uses the + * input index and the relative position of field in input rowType. + * + * <p> For example, if the inputs to a join are + * + * <ul> + * <li>Input #0: EMP(EMPNO, ENAME, DEPTNO) and</li> + * <li>Input #1: DEPT(DEPTNO AS DEPTNO2, DNAME)</li> + * </ul> + * + * <p>then the fields are: + * + * <ul> + * <li>Node #0, Field #0: EMPNO</li> + * <li>Node #0, Field #1: ENAME</li> + * <li>Node #0, Field #2: DEPTNO (from EMP)</li> + * <li>Node #1, Field #0: DEPTNO2 (from DEPT)</li> + * <li>Node #1, Field #1: DNAME</li> + * </ul> + * + * <p> If in some cases, the inputs order of a relation is frequently adjusted and it is difficult + * to maintain the correct RexInputRef, you can consider temporarily replacing RexInputRef with + * RexNodeAndFieldIndex. + * + * @see org.apache.calcite.rel.rules.JoinToHyperGraphRule + * @see org.apache.calcite.rel.rules.HyperGraph#extractJoinCond + * @see org.apache.calcite.rel.rules.HyperEdge#createHyperEdgesFromJoinConds + */ +public class RexNodeAndFieldIndex extends RexVariable { Review Comment: This comment is really good for me! And why name it RexNodeAndFieldIndex? It looks a bit strange. How about HyperGraphInputRef? ########## core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml: ########## @@ -1643,6 +1670,33 @@ LogicalProject(EMPNO=[$9]) LogicalJoin(condition=[=($3, $0)], joinType=[inner]) LogicalTableScan(table=[[CATALOG, SALES, EMP_ADDRESS]]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> + <TestCase name="testComplexPredicateForDphyp"> + <Resource name="sql"> + <![CDATA[select emp.empno from emp, emp_b, dept, dept_nested where emp.deptno + emp_b.empno = dept.deptno + dept_nested.deptno and (emp.empno = emp_b.empno or emp.ename = emp_b.ename) and sqrt(dept.deptno + dept_nested.deptno) > 2]]> + </Resource> + <Resource name="planBefore"> + <![CDATA[ +LogicalProject(EMPNO=[$0]) + HyperGraph(edges=[{0, 1, 2}——[INNER, =(+(node(0)_field(7), node(1)_field(0)), +(node(2)_field(0), node(3)_field(0)))]——{3},{2}——[INNER, >(POWER(+(node(2)_field(0), node(3)_field(0)), 0.5:DECIMAL(2, 1)), CAST(2):DOUBLE NOT NULL)]——{3},{0, 1}——[INNER, true]——{2},{0}——[INNER, OR(=(node(0)_field(0), node(1)_field(0)), =(node(0)_field(1), node(1)_field(1)))]——{1}]) Review Comment: The test results look good. Is there a way we can make the plan appear more concise? ########## core/src/main/java/org/apache/calcite/rel/rules/JoinToHyperGraphRule.java: ########## @@ -55,119 +53,146 @@ protected JoinToHyperGraphRule(Config config) { final Join origJoin = call.rel(0); final RelNode left = call.rel(1); final RelNode right = call.rel(2); - if (origJoin.getJoinType() != JoinRelType.INNER) { + if (unSupportedJoinType(origJoin.getJoinType())) { Review Comment: I prefer using supportedJoinType, although the number of supported types far exceeds the unsupported ones. However, in the future when someone adds joinType, they might not consider this aspect. -- 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]
