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 2019/12/30 06:56:58 UTC
[GitHub] [calcite] danny0405 opened a new pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
danny0405 opened a new pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707
…if original node is transformed to different kind
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362143605
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -396,15 +396,60 @@ public static void verifyTypeEquivalence(
* Copy the {@link org.apache.calcite.rel.hint.RelHint}s from {@code originalRel}
* to {@code newRel} if both of them are {@link Hintable}.
*
- * <p>The hints are filtered by the hint strategies when attaching to the {@code newRel}.
+ * <p>The two relational expressions are assumed as semantically equivalent,
+ * that means the hints should be attached to the relational expression
+ * that expects to have them.
+ *
+ * <p>In order to achieve this, we do the following now:
+ *
+ * <ol>
+ * <li>If the {@code originalRel} is not Project but {@code newRel} is,
+ * try to copy the hints to the {@code newRel}'s input, this is needed because
Review comment:
which `is` ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362137407
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -396,15 +396,60 @@ public static void verifyTypeEquivalence(
* Copy the {@link org.apache.calcite.rel.hint.RelHint}s from {@code originalRel}
* to {@code newRel} if both of them are {@link Hintable}.
*
- * <p>The hints are filtered by the hint strategies when attaching to the {@code newRel}.
+ * <p>The two relational expressions are assumed as semantically equivalent,
+ * that means the hints should be attached to the relational expression
+ * that expects to have them.
+ *
+ * <p>In order to achieve this, we do the following now:
+ *
+ * <ol>
+ * <li>If the {@code originalRel} is not Project but {@code newRel} is,
+ * try to copy the hints to the {@code newRel}'s input, this is needed because
Review comment:
`is` seems redundant.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 merged pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 merged pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362078231
##########
File path: core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
##########
@@ -387,6 +389,25 @@ protected DiffRepository getDiffRepos() {
Collections.emptyList(), Collections.emptyList());
}
+ @Test public void testHintsPropagateWithDifferentKindOfRels() {
+ final String sql = "select /*+ AGG_STRATEGY(TWO_PHASE) */\n"
+ + "ename, avg(sal)\n"
+ + "from emp group by ename";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final RelHint hint = RelHint.of(
+ Collections.emptyList(),
+ "AGG_STRATEGY",
+ Collections.singletonList("TWO_PHASE"));
+ // Validate Hep planner.
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateReduceFunctionsRule.INSTANCE)
Review comment:
Maybe write a mock rule (similar to `MockJoin`) to simulate the `Project + Aggregate` split that you want to test?
I had a hard time to understand why `AggregateReduceFunctionsRule` will trigger what expects be tested. Another benefit is this test will be stable even if when changes are made on `AggregateReduceFunctionsRule`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362143555
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -2495,12 +2504,6 @@ public RelBuilder permute(Mapping mapping) {
return project(exprList);
}
- public RelBuilder aggregate(GroupKey groupKey,
- List<AggregateCall> aggregateCalls) {
- return aggregate(groupKey,
- Lists.transform(aggregateCalls, AggCallImpl2::new));
- }
-
/** Creates a {@link Match}. */
Review comment:
Not that necessary, just move the code where it should belong.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362141663
##########
File path: core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
##########
@@ -387,6 +389,25 @@ protected DiffRepository getDiffRepos() {
Collections.emptyList(), Collections.emptyList());
}
+ @Test public void testHintsPropagateWithDifferentKindOfRels() {
+ final String sql = "select /*+ AGG_STRATEGY(TWO_PHASE) */\n"
+ + "ename, avg(sal)\n"
+ + "from emp group by ename";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final RelHint hint = RelHint.of(
+ Collections.emptyList(),
+ "AGG_STRATEGY",
+ Collections.singletonList("TWO_PHASE"));
+ // Validate Hep planner.
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateReduceFunctionsRule.INSTANCE)
Review comment:
gotcha. Thanks for your clarification.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362133095
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -396,15 +396,60 @@ public static void verifyTypeEquivalence(
* Copy the {@link org.apache.calcite.rel.hint.RelHint}s from {@code originalRel}
* to {@code newRel} if both of them are {@link Hintable}.
*
- * <p>The hints are filtered by the hint strategies when attaching to the {@code newRel}.
+ * <p>The two relational expressions are assumed as semantically equivalent,
+ * that means the hints should be attached to the relational expression
+ * that expect to have them.
Review comment:
Yeah, 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
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362137669
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -2495,12 +2504,6 @@ public RelBuilder permute(Mapping mapping) {
return project(exprList);
}
- public RelBuilder aggregate(GroupKey groupKey,
- List<AggregateCall> aggregateCalls) {
- return aggregate(groupKey,
- Lists.transform(aggregateCalls, AggCallImpl2::new));
- }
-
/** Creates a {@link Match}. */
Review comment:
Is this change expected? It seems not necessary.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
cshuo commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#issuecomment-569847100
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
With regards,
Apache Git Services
[GitHub] [calcite] cshuo commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
cshuo commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#issuecomment-569847280
BTW, I noticed that the docs in `hint/package-info.java` should be updated 😄.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362078231
##########
File path: core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
##########
@@ -387,6 +389,25 @@ protected DiffRepository getDiffRepos() {
Collections.emptyList(), Collections.emptyList());
}
+ @Test public void testHintsPropagateWithDifferentKindOfRels() {
+ final String sql = "select /*+ AGG_STRATEGY(TWO_PHASE) */\n"
+ + "ename, avg(sal)\n"
+ + "from emp group by ename";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final RelHint hint = RelHint.of(
+ Collections.emptyList(),
+ "AGG_STRATEGY",
+ Collections.singletonList("TWO_PHASE"));
+ // Validate Hep planner.
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateReduceFunctionsRule.INSTANCE)
Review comment:
Maybe write a mock rule (similar to `MockJoin`) to simulate the `Project + Aggregate` split that you want to test?
I had a hard time to understand why `AggregateReduceFunctionsRule` will trigger what expects be tested?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362078465
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -396,15 +396,60 @@ public static void verifyTypeEquivalence(
* Copy the {@link org.apache.calcite.rel.hint.RelHint}s from {@code originalRel}
* to {@code newRel} if both of them are {@link Hintable}.
*
- * <p>The hints are filtered by the hint strategies when attaching to the {@code newRel}.
+ * <p>The two relational expressions are assumed as semantically equivalent,
+ * that means the hints should be attached to the relational expression
+ * that expect to have them.
Review comment:
typo? maybe `expects or is 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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#discussion_r362133031
##########
File path: core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
##########
@@ -387,6 +389,25 @@ protected DiffRepository getDiffRepos() {
Collections.emptyList(), Collections.emptyList());
}
+ @Test public void testHintsPropagateWithDifferentKindOfRels() {
+ final String sql = "select /*+ AGG_STRATEGY(TWO_PHASE) */\n"
+ + "ename, avg(sal)\n"
+ + "from emp group by ename";
+ final RelNode rel = tester.convertSqlToRel(sql).rel;
+ final RelHint hint = RelHint.of(
+ Collections.emptyList(),
+ "AGG_STRATEGY",
+ Collections.singletonList("TWO_PHASE"));
+ // Validate Hep planner.
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateReduceFunctionsRule.INSTANCE)
Review comment:
The aim is to test the real rules can propagate the hints successfully. If you also find any bad case, welcome to contribute more.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1707: [CALCITE-3649] Hints should be propagated correctly in planner rules …
URL: https://github.com/apache/calcite/pull/1707#issuecomment-569858323
> BTW, I noticed that the docs in `hint/package-info.java` should be updated 😄.
Yes, thanks for the reminder ~
----------------------------------------------------------------
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
With regards,
Apache Git Services