You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2024/01/18 23:06:04 UTC
[PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
snuyanzin opened a new pull request, #24143:
URL: https://github.com/apache/flink/pull/24143
## What is the purpose of the change
The PR migrates `RewriteMinusAllRule` to java
it doesn't touch `RewriteMinusAllRuleTest` to be sure that java version continues passing it
## Verifying this change
This change is already covered by existing tests
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): ( no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
- The serializers: ( no)
- The runtime per-record code paths (performance sensitive): ( no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no)
- The S3 file system connector: ( no)
## Documentation
- Does this pull request introduce a new feature? ( no)
- If yes, how is the feature documented? (not applicable)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #24143:
URL: https://github.com/apache/flink/pull/24143
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "liuyongvs (via GitHub)" <gi...@apache.org>.
liuyongvs commented on code in PR #24143:
URL: https://github.com/apache/flink/pull/24143#discussion_r1515379811
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.flink.table.planner.plan.rules.logical;
+
+import org.apache.flink.table.planner.plan.utils.SetOpRewriteUtil;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Minus;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.Util;
+import org.immutables.value.Value;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.calcite.sql.type.SqlTypeName.BIGINT;
+import static org.apache.flink.table.planner.functions.sql.FlinkSqlOperatorTable.GREATER_THAN;
+
+/**
+ * Replaces logical [[Minus]] operator using a combination of union all, aggregate and table
+ * function.
+ *
Review Comment:
scala link is different with java @{link}
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #24143:
URL: https://github.com/apache/flink/pull/24143#issuecomment-1899363542
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "24534c846740a8abf784616925b68b4418d1db16",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "24534c846740a8abf784616925b68b4418d1db16",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 24534c846740a8abf784616925b68b4418d1db16 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on code in PR #24143:
URL: https://github.com/apache/flink/pull/24143#discussion_r1543215856
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.flink.table.planner.plan.rules.logical;
+
+import org.apache.flink.table.planner.plan.utils.SetOpRewriteUtil;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Minus;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.Util;
+import org.immutables.value.Value;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.calcite.sql.type.SqlTypeName.BIGINT;
+import static org.apache.flink.table.planner.functions.sql.FlinkSqlOperatorTable.GREATER_THAN;
+
+/**
+ * Replaces logical {@link Minus} operator using a combination of union all, aggregate and table
+ * function.
+ *
+ * <p>Original Query : {@code SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2 }
+ *
+ * <p>Rewritten Query: {@code SELECT c1 FROM ( SELECT c1, sum_val FROM ( SELECT c1, sum(vcol_marker)
+ * AS sum_val FROM ( SELECT c1, 1L as vcol_marker FROM ut1 UNION ALL SELECT c1, -1L as vcol_marker
+ * FROM ut2 ) AS union_all GROUP BY union_all.c1 ) WHERE sum_val > 0 ) LATERAL
+ * TABLE(replicate_row(sum_val, c1)) AS T(c1) }
+ *
+ * <p>Only handle the case of input size 2.
+ */
+@Value.Enclosing
+public class RewriteMinusAllRule extends RelRule<RewriteMinusAllRule.RewriteMinusAllRuleConfig> {
+ public static final RewriteMinusAllRule INSTANCE = RewriteMinusAllRuleConfig.DEFAULT.toRule();
+
+ protected RewriteMinusAllRule(RewriteMinusAllRuleConfig config) {
+ super(config);
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+ Minus minus = call.rel(0);
+ return minus.all && minus.getInputs().size() == 2;
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
+ Minus minus = call.rel(0);
+ RelNode left = minus.getInput(0);
+ RelNode right = minus.getInput(1);
+
+ List<Integer> fields = Util.range(minus.getRowType().getFieldCount());
+
+ // 1. add vcol_marker to left rel node
+ RelBuilder leftBuilder = call.builder().push(left);
Review Comment:
You've slightly changed the variable assignment here from the scala version (the left builder originally didn't include the `.push(left)`). This is fine since the push method returns the same instance, but it should probably be consistent below with the right side.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #24143:
URL: https://github.com/apache/flink/pull/24143#discussion_r1543794924
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.flink.table.planner.plan.rules.logical;
+
+import org.apache.flink.table.planner.plan.utils.SetOpRewriteUtil;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Minus;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.Util;
+import org.immutables.value.Value;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.calcite.sql.type.SqlTypeName.BIGINT;
+import static org.apache.flink.table.planner.functions.sql.FlinkSqlOperatorTable.GREATER_THAN;
+
+/**
+ * Replaces logical {@link Minus} operator using a combination of union all, aggregate and table
+ * function.
+ *
+ * <p>Original Query : {@code SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2 }
Review Comment:
done, thanks
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "RyanSkraba (via GitHub)" <gi...@apache.org>.
RyanSkraba commented on code in PR #24143:
URL: https://github.com/apache/flink/pull/24143#discussion_r1543209301
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.flink.table.planner.plan.rules.logical;
+
+import org.apache.flink.table.planner.plan.utils.SetOpRewriteUtil;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Minus;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.Util;
+import org.immutables.value.Value;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.calcite.sql.type.SqlTypeName.BIGINT;
+import static org.apache.flink.table.planner.functions.sql.FlinkSqlOperatorTable.GREATER_THAN;
+
+/**
+ * Replaces logical {@link Minus} operator using a combination of union all, aggregate and table
+ * function.
+ *
+ * <p>Original Query : {@code SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2 }
Review Comment:
It's not a big deal, but you might want to consider using a `<pre>` block to keep the readable formatting from the original scaladoc. Of course, you might have to escape `>` to `>` :/
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #24143:
URL: https://github.com/apache/flink/pull/24143#discussion_r1543794760
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/RewriteMinusAllRule.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.flink.table.planner.plan.rules.logical;
+
+import org.apache.flink.table.planner.plan.utils.SetOpRewriteUtil;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Minus;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.Util;
+import org.immutables.value.Value;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.apache.calcite.sql.type.SqlTypeName.BIGINT;
+import static org.apache.flink.table.planner.functions.sql.FlinkSqlOperatorTable.GREATER_THAN;
+
+/**
+ * Replaces logical {@link Minus} operator using a combination of union all, aggregate and table
+ * function.
+ *
+ * <p>Original Query : {@code SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2 }
+ *
+ * <p>Rewritten Query: {@code SELECT c1 FROM ( SELECT c1, sum_val FROM ( SELECT c1, sum(vcol_marker)
+ * AS sum_val FROM ( SELECT c1, 1L as vcol_marker FROM ut1 UNION ALL SELECT c1, -1L as vcol_marker
+ * FROM ut2 ) AS union_all GROUP BY union_all.c1 ) WHERE sum_val > 0 ) LATERAL
+ * TABLE(replicate_row(sum_val, c1)) AS T(c1) }
+ *
+ * <p>Only handle the case of input size 2.
+ */
+@Value.Enclosing
+public class RewriteMinusAllRule extends RelRule<RewriteMinusAllRule.RewriteMinusAllRuleConfig> {
+ public static final RewriteMinusAllRule INSTANCE = RewriteMinusAllRuleConfig.DEFAULT.toRule();
+
+ protected RewriteMinusAllRule(RewriteMinusAllRuleConfig config) {
+ super(config);
+ }
+
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+ Minus minus = call.rel(0);
+ return minus.all && minus.getInputs().size() == 2;
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call) {
+ Minus minus = call.rel(0);
+ RelNode left = minus.getInput(0);
+ RelNode right = minus.getInput(1);
+
+ List<Integer> fields = Util.range(minus.getRowType().getFieldCount());
+
+ // 1. add vcol_marker to left rel node
+ RelBuilder leftBuilder = call.builder().push(left);
Review Comment:
This is actually a good catch, thanks, fixed
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-34161][table] Migration of RewriteMinusAllRule to java [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #24143:
URL: https://github.com/apache/flink/pull/24143#issuecomment-2026241663
>A quick question for the Immutable config -- is there a reason or a preference for using the withDescription(...) builders versus description(...)
the main benefit is that with use of `description` we do not need to create an extra object which is created while using `withDescription`, changed to use `description`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org