alex-plekhanov commented on code in PR #12159:
URL: https://github.com/apache/ignite/pull/12159#discussion_r2180211369
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -106,9 +108,12 @@ public enum PlannerPhase {
RuleSets.ofList(
FilterScanMergeRule.TABLE_SCAN_SKIP_CORRELATED,
+ CoreRules.FILTER_EXPAND_IS_NOT_DISTINCT_FROM,
CoreRules.FILTER_MERGE,
CoreRules.FILTER_AGGREGATE_TRANSPOSE,
CoreRules.FILTER_SET_OP_TRANSPOSE,
+ CoreRules.FILTER_SORT_TRANSPOSE,
+ CoreRules.JOIN_CONDITION_EXPAND_IS_NOT_DISTINCT_FROM,
Review Comment:
The same as for FILTER_EXPAND_IS_NOT_DISTINCT_FROM, IS_NOT_DISTINCT_FROM can
be pushed down from join to filter
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -227,11 +235,21 @@ public enum PlannerPhase {
b.operand(LogicalSort.class)
.anyInputs()).toRule(),
+
SetOpToFilterRule.Config.UNION.withDescription("UnionFilterToFilter").toRule(),
Review Comment:
1. Collides with OrToUnion rule
2. Looks like useless in Ignite (all these SetOpToFilter rules), since we
merge filter (and also projects) to table scans and inputs for query with UNION
of the same table, but with different rules will be different and rule will
never match. It can be rewritten with the new rules, thats take into account or
table scans with filters, but not by this ticket.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -227,11 +235,21 @@ public enum PlannerPhase {
b.operand(LogicalSort.class)
.anyInputs()).toRule(),
+
SetOpToFilterRule.Config.UNION.withDescription("UnionFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("IntersectFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("MinusFilterToFilter").toRule(),
CoreRules.UNION_MERGE,
+ CoreRules.UNION_REMOVE,
CoreRules.MINUS_MERGE,
+ CoreRules.MINUS_REMOVE,
+ //CoreRules.INTERSECT_TO_EXISTS,
CoreRules.INTERSECT_MERGE,
- CoreRules.UNION_REMOVE,
+ CoreRules.INTERSECT_REMOVE,
+ CoreRules.INTERSECT_REORDER,
+ //CoreRules.AGGREGATE_MIN_MAX_TO_LIMIT,
Review Comment:
Delete or uncomment
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/SetOpPlannerTest.java:
##########
@@ -304,13 +304,13 @@ public void testSetOpAffinityNested() throws Exception {
")";
assertPlan(sql, publicSchema, isInstanceOf(IgniteExchange.class)
- .and(input(isInstanceOf(setOp.colocated)
- .and(input(0, isTableScan("affinity_tbl2")))
- .and(input(1, isInstanceOf(setOp.colocated)
- .and(input(0, isTableScan("affinity_tbl1")))
- .and(input(1, isTableScan("affinity_tbl2")))
- ))
- )),
+ .and(input(isInstanceOf(setOp.colocated)
+ .and(inputCommuted(isTableScan("affinity_tbl2")))
+ .and(inputCommuted(isInstanceOf(setOp.colocated)
+ .and(input(0, isTableScan("affinity_tbl1")))
+ .and(input(1, isTableScan("affinity_tbl2")))
+ ))
+ )),
Review Comment:
Wrong indent
`inputCommuted` -> `hasChildThat`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -106,9 +108,12 @@ public enum PlannerPhase {
RuleSets.ofList(
FilterScanMergeRule.TABLE_SCAN_SKIP_CORRELATED,
+ CoreRules.FILTER_EXPAND_IS_NOT_DISTINCT_FROM,
Review Comment:
We can exploit IS_NOT_DISTINCT_FROM for index bounds, and it's not working
for complex conditions like `CASE ...`. We fully support IS_NOT_DISTINCT_FROM
in filtering, hash spools, etc.. I think this rule makes more harm than helps.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -634,6 +634,22 @@ protected <T extends RelNode> Predicate<RelNode> input(int
idx, Predicate<T> pre
};
}
+ /**
+ * Predicate builder for "Any input satisfy predicate" condition.
+ */
+ protected <T extends RelNode> Predicate<RelNode>
inputCommuted(Predicate<T> predicate) {
Review Comment:
Looks like can be effectevily replaced with `hashChildThat`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -227,11 +235,21 @@ public enum PlannerPhase {
b.operand(LogicalSort.class)
.anyInputs()).toRule(),
+
SetOpToFilterRule.Config.UNION.withDescription("UnionFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("IntersectFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("MinusFilterToFilter").toRule(),
Review Comment:
INTERSECT twice
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -181,6 +186,9 @@ public enum PlannerPhase {
JoinPushExpressionsRule.Config.DEFAULT
.withOperandFor(LogicalJoin.class).toRule(),
+
ExpandDisjunctionForTableRule.Config.FILTER.withDescription("ExpandFilterDisjunctionGlobal").toRule(),
+
ExpandDisjunctionForTableRule.Config.FILTER.withDescription("ExpandJoinDisjunctionGlobal").toRule(),
Review Comment:
The same rule twice
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerPhase.java:
##########
@@ -227,11 +235,21 @@ public enum PlannerPhase {
b.operand(LogicalSort.class)
.anyInputs()).toRule(),
+
SetOpToFilterRule.Config.UNION.withDescription("UnionFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("IntersectFilterToFilter").toRule(),
+
SetOpToFilterRule.Config.INTERSECT.withDescription("MinusFilterToFilter").toRule(),
CoreRules.UNION_MERGE,
+ CoreRules.UNION_REMOVE,
CoreRules.MINUS_MERGE,
+ CoreRules.MINUS_REMOVE,
+ //CoreRules.INTERSECT_TO_EXISTS,
Review Comment:
Delete or uncomment.Maybe useful.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/SetOpPlannerTest.java:
##########
@@ -368,8 +368,8 @@ public void testSetOpBroadcastAndRandomNested() throws
Exception {
")";
assertPlan(sql, publicSchema, isInstanceOf(setOp.colocated)
- .and(input(0, isTableScan("broadcast_tbl1")))
- .and(input(1, isInstanceOf(setOp.reduce)
+ .and(inputCommuted(isTableScan("broadcast_tbl1")))
+ .and(inputCommuted(isInstanceOf(setOp.reduce)
Review Comment:
`inputCommuted` -> `hasChildThat`
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
