alex-plekhanov commented on code in PR #12284:
URL: https://github.com/apache/ignite/pull/12284#discussion_r2455830094
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/MergeJoinConverterRule.java:
##########
@@ -49,18 +49,20 @@ public MergeJoinConverterRule() {
/** {@inheritDoc} */
@Override public boolean matchesJoin(RelOptRuleCall call) {
- LogicalJoin logicalJoin = call.rel(0);
+ LogicalJoin logicaJoin = call.rel(0);
Review Comment:
`logicaJoin` - typo
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rel;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinInfo;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+
+/** Extended {@link JoinInfo}. */
+public class IgniteJoinInfo extends JoinInfo {
+ /** Conditions with mathing nulls. It usually means presence of 'IS
DISTINCT' / 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
+
+ /** */
+ protected IgniteJoinInfo(
+ ImmutableIntList leftKeys,
+ ImmutableIntList rightKeys,
+ ImmutableBitSet matchingNulls,
+ ImmutableList<RexNode> nonEquis
+ ) {
+ super(leftKeys, rightKeys, nonEquis);
+
+ this.matchingNulls = matchingNulls;
+ }
+
+ /** */
+ public static IgniteJoinInfo of(ImmutableIntList leftKeys,
ImmutableIntList rightKeys) {
+ return new IgniteJoinInfo(leftKeys, rightKeys, ImmutableBitSet.of(),
ImmutableList.of());
+ }
+
+ /** */
+ public static IgniteJoinInfo of(Join join) {
+ List<Integer> leftKeys = new ArrayList<>();
+ List<Integer> rightKeys = new ArrayList<>();
+ List<Boolean> filteredNulls = new ArrayList<>();
Review Comment:
In `splitJoinCondition` this parameter is called `filterNulls`, I think it's
more correct name than `filtered` (also `skipNulls` can be used here)
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rel;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinInfo;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+
+/** Extended {@link JoinInfo}. */
+public class IgniteJoinInfo extends JoinInfo {
+ /** Conditions with mathing nulls. It usually means presence of 'IS
DISTINCT' / 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
+
+ /** */
+ protected IgniteJoinInfo(
+ ImmutableIntList leftKeys,
+ ImmutableIntList rightKeys,
+ ImmutableBitSet matchingNulls,
+ ImmutableList<RexNode> nonEquis
+ ) {
+ super(leftKeys, rightKeys, nonEquis);
+
+ this.matchingNulls = matchingNulls;
+ }
+
+ /** */
+ public static IgniteJoinInfo of(ImmutableIntList leftKeys,
ImmutableIntList rightKeys) {
+ return new IgniteJoinInfo(leftKeys, rightKeys, ImmutableBitSet.of(),
ImmutableList.of());
+ }
Review Comment:
Not used
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteMergeJoin.java:
##########
@@ -86,6 +90,7 @@ public IgniteMergeJoin(RelInput input) {
input.getInputs().get(0),
input.getInputs().get(1),
input.getExpression("condition"),
+ null,
Review Comment:
Value is lost during serialization/deserialization. You should use
`input.getBitSet("allowNulls")` here and `.item("allowNulls", allowNulls)` in
`explainTerms`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java:
##########
@@ -298,13 +300,58 @@ public LogicalRelImplementor(
RelDataType rightType = rel.getRight().getRowType();
JoinRelType joinType = rel.getJoinType();
- int pairsCnt = rel.analyzeCondition().pairs().size();
+ List<IntPair> joinPairs = rel.analyzeCondition().pairs();
+ int pairsCnt = joinPairs.size();
+ int matchingNullsCnt = rel.matchingNulls().isEmpty() ? 0 :
rel.matchingNulls().cardinality();
- Comparator<Row> comp = expressionFactory.comparator(
- rel.leftCollation().getFieldCollations().subList(0, pairsCnt),
- rel.rightCollation().getFieldCollations().subList(0, pairsCnt),
- rel.getCondition().getKind() == IS_NOT_DISTINCT_FROM ||
rel.getCondition().getKind() == IS_DISTINCT_FROM
- );
+ Comparator<Row> comp;
+
+ List<RelFieldCollation> leftCollations =
rel.leftCollation().getFieldCollations().subList(0, pairsCnt);
+ List<RelFieldCollation> rightCollations =
rel.rightCollation().getFieldCollations().subList(0, pairsCnt);
+
+ // No conditions like 'IS NOT DISTINCT' or all the conditions are.
+ if (matchingNullsCnt == 0 || matchingNullsCnt == pairsCnt) {
+ boolean nullsMatch = !rel.matchingNulls().isEmpty();
+
+ comp = expressionFactory.comparator(
+ leftCollations,
+ rightCollations,
+ matchingNullsCnt == 0 ? null : (lidx, ridx) -> nullsMatch
+ );
+ }
+ else {
+ // Maps collations order on join info's pairs order.
+ Map<IntPair, Integer> collsToPairIdxMap = U.newHashMap(pairsCnt);
+
+ for (int c = 0; c < pairsCnt; ++c) {
+ RelFieldCollation leftColl = leftCollations.get(c);
+ RelFieldCollation rightColl = rightCollations.get(c);
+
+ for (int p = 0; p < pairsCnt; ++p) {
+ IntPair pair = joinPairs.get(p);
+
+ if (pair.source == leftColl.getFieldIndex() && pair.target
== rightColl.getFieldIndex()) {
+ collsToPairIdxMap.put(pair, p);
+
+ break;
+ }
+ }
+ }
+
+ assert collsToPairIdxMap.size() == pairsCnt;
+
+ comp = expressionFactory.comparator(
+ leftCollations,
+ rightCollations,
+ (lidx, ridx) -> {
+ Integer pairIdx = collsToPairIdxMap.get(new IntPair(lidx,
ridx));
+
+ assert pairIdx != null;
+
+ return rel.matchingNulls().get(pairIdx);
+ }
+ );
+ }
Review Comment:
1. There can be semi-duplicated conditions (`equals` and the same condition
with `not distinct from`), in this case collations can have smaller size than
pairs and their will be error on `subList` method execution.
2. Looks overcomplicated, map is redundant and resource consuming, lambdas
is hard to read. Let's use remapped BitSet instead of map/lambda and use it in
comparator builder.
Proposed implementation:
```
List<IntPair> joinPairs = rel.analyzeCondition().pairs();
int pairsCnt = joinPairs.size();
List<RelFieldCollation> leftCollations =
rel.leftCollation().getFieldCollations();
List<RelFieldCollation> rightCollations =
rel.rightCollation().getFieldCollations();
ImmutableBitSet allowNulls = rel.allowNulls();
ImmutableBitSet.Builder collsAllowNullsBuilder =
ImmutableBitSet.builder();
int lastCollField = -1;
for (int c = 0; c < Math.min(leftCollations.size(),
rightCollations.size()); ++c) {
RelFieldCollation leftColl = leftCollations.get(c);
RelFieldCollation rightColl = rightCollations.get(c);
collsAllowNullsBuilder.set(c);
for (int p = 0; p < pairsCnt; ++p) {
IntPair pair = joinPairs.get(p);
if (pair.source == leftColl.getFieldIndex() && pair.target
== rightColl.getFieldIndex()) {
lastCollField = c;
if (!allowNulls.get(p)) {
collsAllowNullsBuilder.clear(c);
break;
}
}
}
}
Comparator<Row> comp = expressionFactory.comparator(
leftCollations.subList(0, lastCollField + 1),
rightCollations.subList(0, lastCollField + 1),
collsAllowNullsBuilder.build()
);
```
Comparator can be simplified as well, only required change is in nulls
comparison condition:
```
if (c1 == null && c2 == null) {
if (!allowNulls.get(i))
hasNulls = true;
continue;
}
```
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/JoinIntegrationTest.java:
##########
@@ -65,6 +65,42 @@ public static List<Object[]> params() {
// NO-OP
}
+ /** */
+ @Test
+ public void testIsNotDistinctWithEquiConditionFrom() {
+ // 'IS NOT DISTINCT' is first.
+ assertQuery("select t1.c2, t1.c3, t2.c2, t2.c3 from t1 join t2 on
t1.c2 is not distinct from t2.c2 and t1.c3 = t2.c3 - 1")
+ .returns(null, 2, null, 3)
+ .check();
+
+ // 'IS NOT DISTINCT' is second.
+ assertQuery("select t1.c2, t1.c3, t2.c2, t2.c3 from t1 join t2 on
t1.c3 = t2.c3 - 1 and t1.c2 is not distinct from t2.c2")
+ .returns(null, 2, null, 3)
+ .check();
+
+ // Duplicated condition.
+ assertQuery("select t1.c2, t1.c3, t2.c2, t2.c3 from t1 join t2 on
t1.c2 is not distinct from t2.c2 " +
+ "and t1.c2 is not distinct from t2.c2 and t1.c3 = t2.c3 - 1")
+ .returns(null, 2, null, 3)
+ .check();
+ assertQuery("select t1.c2, t1.c3, t2.c2, t2.c3 from t1 join t2 on
t1.c3 = t2.c3 - 1 and t1.c3 = t2.c3 - 1" +
+ "and t1.c2 is not distinct from t2.c2 and t1.c2 is not distinct
from t2.c2")
+ .returns(null, 2, null, 3)
+ .check();
+
Review Comment:
Let's add test:
```
assertQuery("select t1.c2, t1.c3, t2.c2, t2.c3 from t1 join t2 " +
"on t1.c2 is not distinct from t2.c2 and t1.c2 = t2.c2 and t1.c3
= t2.c3 and t1.c3 is not distinct from t2.c3")
.returns(1, 1, 1, 1)
.returns(2, 2, 2, 2)
.returns(3, 3, 3, 3)
.returns(4, 4, 4, 4)
.check();
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rel;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinInfo;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+
+/** Extended {@link JoinInfo}. */
+public class IgniteJoinInfo extends JoinInfo {
+ /** Conditions with mathing nulls. It usually means presence of 'IS
DISTINCT' / 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
Review Comment:
`matchingNulls` also not a very good naming. Maybe something like
`allowNulls`?
`allowNulls` is also used in `IgniteHashIndexSpool`, `RuntimeHashIndex`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rel;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinInfo;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+
+/** Extended {@link JoinInfo}. */
+public class IgniteJoinInfo extends JoinInfo {
+ /** Conditions with mathing nulls. It usually means presence of 'IS
DISTINCT' / 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
+
+ /** */
+ protected IgniteJoinInfo(
+ ImmutableIntList leftKeys,
+ ImmutableIntList rightKeys,
+ ImmutableBitSet matchingNulls,
+ ImmutableList<RexNode> nonEquis
+ ) {
+ super(leftKeys, rightKeys, nonEquis);
+
+ this.matchingNulls = matchingNulls;
+ }
+
+ /** */
+ public static IgniteJoinInfo of(ImmutableIntList leftKeys,
ImmutableIntList rightKeys) {
+ return new IgniteJoinInfo(leftKeys, rightKeys, ImmutableBitSet.of(),
ImmutableList.of());
+ }
+
+ /** */
+ public static IgniteJoinInfo of(Join join) {
+ List<Integer> leftKeys = new ArrayList<>();
+ List<Integer> rightKeys = new ArrayList<>();
+ List<Boolean> filteredNulls = new ArrayList<>();
+ List<RexNode> nonEquis = new ArrayList<>();
+
+ RelOptUtil.splitJoinCondition(join.getLeft(), join.getRight(),
join.getCondition(), leftKeys, rightKeys,
+ filteredNulls, nonEquis);
+
+ Collection<Integer> matchingNulls = null;
+
+ for (int i = 0; i < filteredNulls.size(); ++i) {
+ if (!filteredNulls.get(i)) {
+ if (matchingNulls == null)
+ matchingNulls = new ArrayList<>(filteredNulls.size());
+
+ matchingNulls.add(i);
+ }
+ }
Review Comment:
```
ImmutableBitSet.Builder matchingNulls = ImmutableBitSet.builder();
for (int i = 0; i < filteredNulls.size(); ++i) {
if (!filteredNulls.get(i))
matchingNulls.set(i);
}
```
and than `matchingNulls.build()`
?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteMergeJoin.java:
##########
@@ -64,17 +64,21 @@ public class IgniteMergeJoin extends AbstractIgniteJoin {
*/
private final RelCollation rightCollation;
+ /** Mathing null conditions like 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
Review Comment:
Let's rename to `allowNulls`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteJoinInfo.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rel;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinInfo;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+
+/** Extended {@link JoinInfo}. */
+public class IgniteJoinInfo extends JoinInfo {
+ /** Conditions with mathing nulls. It usually means presence of 'IS
DISTINCT' / 'IS NOT DISTINCT'. */
+ private final ImmutableBitSet matchingNulls;
+
+ /** */
+ protected IgniteJoinInfo(
+ ImmutableIntList leftKeys,
+ ImmutableIntList rightKeys,
+ ImmutableBitSet matchingNulls,
+ ImmutableList<RexNode> nonEquis
+ ) {
+ super(leftKeys, rightKeys, nonEquis);
+
+ this.matchingNulls = matchingNulls;
+ }
+
+ /** */
+ public static IgniteJoinInfo of(ImmutableIntList leftKeys,
ImmutableIntList rightKeys) {
+ return new IgniteJoinInfo(leftKeys, rightKeys, ImmutableBitSet.of(),
ImmutableList.of());
+ }
+
+ /** */
+ public static IgniteJoinInfo of(Join join) {
+ List<Integer> leftKeys = new ArrayList<>();
+ List<Integer> rightKeys = new ArrayList<>();
+ List<Boolean> filteredNulls = new ArrayList<>();
+ List<RexNode> nonEquis = new ArrayList<>();
+
+ RelOptUtil.splitJoinCondition(join.getLeft(), join.getRight(),
join.getCondition(), leftKeys, rightKeys,
+ filteredNulls, nonEquis);
+
+ Collection<Integer> matchingNulls = null;
+
+ for (int i = 0; i < filteredNulls.size(); ++i) {
+ if (!filteredNulls.get(i)) {
+ if (matchingNulls == null)
+ matchingNulls = new ArrayList<>(filteredNulls.size());
+
+ matchingNulls.add(i);
+ }
+ }
+
+ return new IgniteJoinInfo(
+ ImmutableIntList.of(leftKeys.stream().mapToInt(i -> i).toArray()),
+ ImmutableIntList.of(rightKeys.stream().mapToInt(i -> i).toArray()),
Review Comment:
ImmutableIntList.copyOf(leftKeys),
ImmutableIntList.copyOf(rightKeys),
--
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]
