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/01/21 00:37:15 UTC
[GitHub] [calcite] hsyuan opened a new pull request #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
hsyuan opened a new pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772
Some times RelSet can get merged. e.g.:
Each number represet Rel id, and they are in different RelSet.
```
1 4
\ /
2 /
\ /
3
```
Assume in the rulequeue, we have rule match with name of 12, 23, 43. For
simplicity, omit the rule name, first digit represent the parent RelNode,
second represent the child.
If after some rule, we merged the set of 3 into the set of 2, and RelNode 4's
input is replaced by RelSubset of 2. We will retrigger rules and try to add
rulematch 12, 42, 43 into rule queue.
12 will be filtered out, because there is duplicate in the RuleMatch names set.
But for 43, it now has different rulematch digest with previous match, because
RelNode # 4's input RelSubset changed, its digest also changed. So we can't
detect the duplication and will re-apply rulematch 43 twice.
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369368258
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -143,18 +142,18 @@ private String computeDigest() {
new StringBuilder("rule [" + getRule() + "] rels [");
for (int i = 0; i < rels.length; i++) {
if (i > 0) {
- buf.append(", ");
+ buf.append(',');
}
Review comment:
It is useless.
----------------------------------------------------------------
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] zhenyu-hou commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
zhenyu-hou commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r368803861
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append("#" + rels[i].getId());
}
buf.append("]");
Review comment:
change to buf.append(']') to improvement perf slightly?
----------------------------------------------------------------
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 #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#issuecomment-576993309
> @vlsi
> SELECT * FROM TABLE(Ramp(3, 4))
> when i enter this query in sqlline in apache calcite i am getting error as
> No match found for function signature RAMP(, ) (state=,code=0)
You can not select from a user_defined table function directly, you can join it with another table.
----------------------------------------------------------------
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 #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369359963
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -494,11 +494,10 @@ VolcanoRuleMatch popMatch(VolcanoPlannerPhase phase) {
}
}
- // A rule match's digest is composed of the operand RelNodes' digests,
- // which may have changed if sets have merged since the rule match was
- // enqueued.
- match.recomputeDigest();
-
+ // If sets have merged since the rule match was enqueued, the match
+ // may not be removed from the matchMap because the subset may have
+ // changed, it is OK to leave it since the matchMap will be cleared
+ // at the end.
phaseMatchList.matchMap.remove(
Review comment:
The change does not fix the title, right ?
----------------------------------------------------------------
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] swathi858 edited a comment on issue #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
swathi858 edited a comment on issue #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#issuecomment-577618418
when i download apache calcite tar file from https://calcite.apache.org/downloads/ and when i run sqlline.bat in windows i am getting error as
BUILD SUCCESSFUL in 3m 55s
57 actionable tasks: 48 executed, 9 up-to-date
Error: Could not find or load main class sqlline.SqlLine
----------------------------------------------------------------
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] swathi858 edited a comment on issue #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
swathi858 edited a comment on issue #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#issuecomment-576523215
@vlsi
SELECT * FROM TABLE(Ramp(3, 4))
when i enter this query in sqlline in apache calcite i am getting error as
No match found for function signature RAMP(<NUMERIC>, <NUMERIC>) (state=,code=0)
----------------------------------------------------------------
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] zhenyu-hou commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
zhenyu-hou commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r368803761
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append("#" + rels[i].getId());
Review comment:
how about buf.append('#').append(rels[i].getId());
----------------------------------------------------------------
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 #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369360120
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -143,18 +142,18 @@ private String computeDigest() {
new StringBuilder("rule [" + getRule() + "] rels [");
for (int i = 0; i < rels.length; i++) {
if (i > 0) {
- buf.append(", ");
+ buf.append(',');
}
Review comment:
why remove the whitespace ?
----------------------------------------------------------------
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 #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369360224
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -143,18 +142,18 @@ private String computeDigest() {
new StringBuilder("rule [" + getRule() + "] rels [");
for (int i = 0; i < rels.length; i++) {
if (i > 0) {
- buf.append(", ");
+ buf.append(',');
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append('#').append(rels[i].getId());
}
- buf.append("]");
+ buf.append(']');
return buf.toString();
Review comment:
This is not a Java code.
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369195618
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
Review comment:
done
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369195780
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append("#" + rels[i].getId());
Review comment:
done
----------------------------------------------------------------
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] vlsi commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r368827225
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -494,11 +494,10 @@ VolcanoRuleMatch popMatch(VolcanoPlannerPhase phase) {
}
}
- // A rule match's digest is composed of the operand RelNodes' digests,
- // which may have changed if sets have merged since the rule match was
- // enqueued.
- match.recomputeDigest();
-
+ // If sets have merged since the rule match was enqueued, the match
+ // may not be removed from the matchMap because the subset may have
+ // changed, it is OK to leave it since the matchMap will be cleared
+ // at the end.
phaseMatchList.matchMap.remove(
Review comment:
Can we remove the relevant entry when subsets are merged/removed?
Otherwise, it looks like a memory leak.
----------------------------------------------------------------
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 #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369360224
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -143,18 +142,18 @@ private String computeDigest() {
new StringBuilder("rule [" + getRule() + "] rels [");
for (int i = 0; i < rels.length; i++) {
if (i > 0) {
- buf.append(", ");
+ buf.append(',');
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append('#').append(rels[i].getId());
}
- buf.append("]");
+ buf.append(']');
return buf.toString();
Review comment:
This is not a Java code.
----------------------------------------------------------------
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] zhenyu-hou commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
zhenyu-hou commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r368803861
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append("#" + rels[i].getId());
}
buf.append("]");
Review comment:
similarly, change to buf.append(']') to improve perf slightly?
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369368609
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -494,11 +494,10 @@ VolcanoRuleMatch popMatch(VolcanoPlannerPhase phase) {
}
}
- // A rule match's digest is composed of the operand RelNodes' digests,
- // which may have changed if sets have merged since the rule match was
- // enqueued.
- match.recomputeDigest();
-
+ // If sets have merged since the rule match was enqueued, the match
+ // may not be removed from the matchMap because the subset may have
+ // changed, it is OK to leave it since the matchMap will be cleared
+ // at the end.
phaseMatchList.matchMap.remove(
Review comment:
What do you mean by `not fix the title`?
----------------------------------------------------------------
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] zhenyu-hou commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
zhenyu-hou commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r368803682
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
Review comment:
as we are changing how the digest is computed anyway, we can simplify this as buf.append(',');
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369271502
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RuleQueue.java
##########
@@ -494,11 +494,10 @@ VolcanoRuleMatch popMatch(VolcanoPlannerPhase phase) {
}
}
- // A rule match's digest is composed of the operand RelNodes' digests,
- // which may have changed if sets have merged since the rule match was
- // enqueued.
- match.recomputeDigest();
-
+ // If sets have merged since the rule match was enqueued, the match
+ // may not be removed from the matchMap because the subset may have
+ // changed, it is OK to leave it since the matchMap will be cleared
+ // at the end.
phaseMatchList.matchMap.remove(
Review comment:
You have to remove all the matches for the subset in `matchMap`, find them in the `rulequeue` and remove them from `rulequeue`, and remove them from `names` set. I don't think it is worth sweating.
Since the leak already exists before this change, I prefer leaving it as it is.
I am going to propose a change to completely remove rulematch importance.
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1772:
[CALCITE-3744] Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#discussion_r369195909
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleMatch.java
##########
@@ -145,16 +144,16 @@ private String computeDigest() {
if (i > 0) {
buf.append(", ");
}
- RelOptUtil.appendRelDescription(buf, rels[i]);
+ buf.append("#" + rels[i].getId());
}
buf.append("]");
Review comment:
done
----------------------------------------------------------------
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] swathi858 commented on issue #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
swathi858 commented on issue #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#issuecomment-576523215
SELECT * FROM TABLE(Ramp(3, 4))
when i enter this query in sqlline in apache calcite i am getting error as
No match found for function signature RAMP(<NUMERIC>, <NUMERIC>) (state=,code=0)
----------------------------------------------------------------
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] swathi858 commented on issue #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
swathi858 commented on issue #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772#issuecomment-577618418
when i download apache calcite tar file from https://www.apache.org/dyn/closer.lua?filename=calcite/apache-calcite-1.21.0/apache-calcite-1.21.0-src.tar.gz&action=download and when i run sqlline.bat in windows i am getting error as
BUILD SUCCESSFUL in 3m 55s
57 actionable tasks: 48 executed, 9 up-to-date
Error: Could not find or load main class sqlline.SqlLine
----------------------------------------------------------------
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] hsyuan merged pull request #1772: [CALCITE-3744]
Duplicate rule matches when RelSet gets merged
Posted by GitBox <gi...@apache.org>.
hsyuan merged pull request #1772: [CALCITE-3744] Duplicate rule matches when RelSet gets merged
URL: https://github.com/apache/calcite/pull/1772
----------------------------------------------------------------
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