You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/11/08 14:30:44 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #2252: [CALCITE-4386] Support RelShuttle visit specific logical operators
xy2953396112 opened a new pull request #2252:
URL: https://github.com/apache/calcite/pull/2252
https://issues.apache.org/jira/browse/CALCITE-4386
In the current implementation, some operators, such as `LogicalProject` and `LogicalFilter`, can be directly accessed by `Relshuttle`, while others, such as `LogicalCalc` and `LogicalWindow`, cannot be accessed directly. `RelShuttle` is a very important tool class. Usually, we will do equivalent transformation of relational algebra in the process of relational algebra optimization. This PR will cover the method of the operator visit `RelShuttle`, and the specific operator can be accessed directly by visiting `RelNode`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r606969187
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2349,6 +2349,27 @@ public final Sql sql(String sql) {
assertThat(rels.get(0), isA(LogicalCalc.class));
}
+ @Test void testRelShuttleForLogicalCalcShuttle() {
+ final String sql = "select count(ename) from emp";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final HepProgramBuilder programBuilder = HepProgram.builder();
+ programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+ final HepPlanner planner = new HepPlanner(programBuilder.build());
+ planner.setRoot(rel);
+ final RelNode calc = planner.findBestExp();
+ final List<RelNode> rels = new ArrayList<>();
+ final RelShuttleImpl visitor = new RelShuttleImpl() {
+ @Override public RelNode visit(LogicalCalc calc) {
+ RelNode visitedRel = super.visit(calc);
+ rels.add(visitedRel);
+ return visitedRel;
+ }
+ };
+ visitor.visit(calc);
Review comment:
ok
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on pull request #2252: [CALCITE-4386] Support RelShuttle visit specific logical operators
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#issuecomment-735740330
> Commented in JIRA. I don't think this change is necessary.
Please help to review, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#issuecomment-752302121
> You are trying to add RelShuttle support for LogicalCalc, do you?
yes
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r602842213
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2349,6 +2349,27 @@ public final Sql sql(String sql) {
assertThat(rels.get(0), isA(LogicalCalc.class));
}
+ @Test void testRelShuttleForLogicalCalcShuttle() {
+ final String sql = "select count(ename) from emp";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final HepProgramBuilder programBuilder = HepProgram.builder();
+ programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+ final HepPlanner planner = new HepPlanner(programBuilder.build());
+ planner.setRoot(rel);
+ final RelNode calc = planner.findBestExp();
+ final List<RelNode> rels = new ArrayList<>();
+ final RelShuttleImpl visitor = new RelShuttleImpl() {
+ @Override public RelNode visit(LogicalCalc calc) {
+ RelNode visitedRel = super.visit(calc);
+ rels.add(visitedRel);
+ return visitedRel;
+ }
+ };
+ visitor.visit(calc);
+ assertThat(rels.size(), is(1));
Review comment:
Please add message that clarifies what is compared here, and why is 1 expected
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#issuecomment-808856131
OK, LGTM
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 closed pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
xy2953396112 closed pull request #2252:
URL: https://github.com/apache/calcite/pull/2252
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit specific logical operators
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r541924126
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2349,6 +2349,27 @@ public final Sql sql(String sql) {
assertThat(rels.get(0), isA(LogicalCalc.class));
}
+ @Test void testRelShuttleForLogicalCalcShuttle() {
+ final String sql = "select count(ename) from emp";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final HepProgramBuilder programBuilder = HepProgram.builder();
+ programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+ final HepPlanner planner = new HepPlanner(programBuilder.build());
+ planner.setRoot(rel);
+ final RelNode calc = planner.findBestExp();
+ final List<RelNode> rels = new ArrayList<>();
+ final RelShuttleImpl visitor = new RelShuttleImpl() {
+ @Override public RelNode visit(LogicalCalc calc) {
+ RelNode visitedRel = super.visit(calc);
+ rels.add(visitedRel);
+ return visitedRel;
+ }
+ };
+ visitor.visit(calc);
+ assertThat(rels.size(), is(1));
+ assertThat(rels.get(0), isA(LogicalCalc.class));
+ }
+
Review comment:
You also add RelShuttle support for some other operators, like LogicalWindow, LogicalTableModify, etc. Can you also add test case for these operators?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on pull request #2252: [CALCITE-4386] Support RelShuttle visit specific logical operators
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#issuecomment-744006383
You are trying to add RelShuttle support for LogicalCalc, do you?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r549912091
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2349,6 +2349,27 @@ public final Sql sql(String sql) {
assertThat(rels.get(0), isA(LogicalCalc.class));
}
+ @Test void testRelShuttleForLogicalCalcShuttle() {
+ final String sql = "select count(ename) from emp";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final HepProgramBuilder programBuilder = HepProgram.builder();
+ programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+ final HepPlanner planner = new HepPlanner(programBuilder.build());
+ planner.setRoot(rel);
+ final RelNode calc = planner.findBestExp();
+ final List<RelNode> rels = new ArrayList<>();
+ final RelShuttleImpl visitor = new RelShuttleImpl() {
+ @Override public RelNode visit(LogicalCalc calc) {
+ RelNode visitedRel = super.visit(calc);
+ rels.add(visitedRel);
+ return visitedRel;
+ }
+ };
+ visitor.visit(calc);
+ assertThat(rels.size(), is(1));
+ assertThat(rels.get(0), isA(LogicalCalc.class));
+ }
+
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r602843814
##########
File path: core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java
##########
@@ -2349,6 +2349,27 @@ public final Sql sql(String sql) {
assertThat(rels.get(0), isA(LogicalCalc.class));
}
+ @Test void testRelShuttleForLogicalCalcShuttle() {
+ final String sql = "select count(ename) from emp";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final HepProgramBuilder programBuilder = HepProgram.builder();
+ programBuilder.addRuleInstance(CoreRules.PROJECT_TO_CALC);
+ final HepPlanner planner = new HepPlanner(programBuilder.build());
+ planner.setRoot(rel);
+ final RelNode calc = planner.findBestExp();
+ final List<RelNode> rels = new ArrayList<>();
+ final RelShuttleImpl visitor = new RelShuttleImpl() {
+ @Override public RelNode visit(LogicalCalc calc) {
+ RelNode visitedRel = super.visit(calc);
+ rels.add(visitedRel);
+ return visitedRel;
+ }
+ };
+ visitor.visit(calc);
Review comment:
use `calc.accept(visitor);`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r726930673
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/ProjectToCalcRule.java
##########
@@ -66,7 +66,7 @@ public ProjectToCalcRule(RelBuilderFactory relBuilderFactory) {
project.getRowType(),
project.getCluster().getRexBuilder());
final LogicalCalc calc = LogicalCalc.create(input, program);
- call.transformTo(calc);
+ call.transformTo(calc.withHints(project.getHints()));
Review comment:
why need this?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #2252: [CALCITE-4386] Support RelShuttle visit LogicalCalc
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #2252:
URL: https://github.com/apache/calcite/pull/2252#discussion_r602844203
##########
File path: core/src/main/java/org/apache/calcite/rel/RelHomogeneousShuttle.java
##########
@@ -56,6 +57,10 @@
return visit((RelNode) values);
}
+ @Override public RelNode visit(LogicalCalc calc) {
+ return visit((RelNode) calc);
+ }
Review comment:
why need this update?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org