[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance URL: https://github.com/apache/calcite/pull/1840#discussion_r389829592 ## File path: core/src/test/resources/sql/winagg.iq ## @@ -431,14 +431,14 @@ join ( from "hr"."emps" window w as (partition by "deptno" order by "commission")) b on a."deptno" = b."deptno" -limit 5; +order by "deptno", ar, br limit 5; Review comment: You can modify the result, instead of modifying the query. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance URL: https://github.com/apache/calcite/pull/1840#discussion_r389440266 ## File path: core/src/test/resources/sql/winagg.iq ## @@ -431,14 +431,14 @@ join ( from "hr"."emps" window w as (partition by "deptno" order by "commission")) b on a."deptno" = b."deptno" -limit 5; +order by "deptno", ar, br limit 5; Review comment: If someone tuned the cost model and caused a plan change, the result will be different again. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389332018
##
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##
@@ -801,6 +801,11 @@ public JdbcSort(
offset, fetch);
}
+@Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+RelMetadataQuery mq) {
+ return super.computeSelfCost(planner, mq).multiplyBy(0.9);
Review comment:
If you don't think the following plans should have different cost, I am
happy to change it back:
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance URL: https://github.com/apache/calcite/pull/1840#discussion_r389331800 ## File path: core/src/test/resources/sql/winagg.iq ## @@ -431,14 +431,14 @@ join ( from "hr"."emps" window w as (partition by "deptno" order by "commission")) b on a."deptno" = b."deptno" -limit 5; +order by "deptno", ar, br limit 5; Review comment: Without ordering, different join order will generate different data. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance URL: https://github.com/apache/calcite/pull/1840#discussion_r389323870 ## File path: core/src/test/resources/sql/winagg.iq ## @@ -431,14 +431,14 @@ join ( from "hr"."emps" window w as (partition by "deptno" order by "commission")) b on a."deptno" = b."deptno" -limit 5; +order by "deptno", ar, br limit 5; Review comment: To stablize test. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389323751
##
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##
@@ -801,6 +801,11 @@ public JdbcSort(
offset, fetch);
}
+@Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+RelMetadataQuery mq) {
+ return super.computeSelfCost(planner, mq).multiplyBy(0.9);
Review comment:
To make it cheaper than default sort. Same applies on `GeodeSort`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388684093
##
File path: core/src/test/resources/sql/sub-query.iq
##
@@ -831,7 +831,7 @@ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t2)],
expr#5=[IS TRUE($t4)], e
EnumerableLimit(fetch=[1])
EnumerableSort(sort0=[$0], dir0=[DESC])
EnumerableAggregate(group=[{0}], c=[COUNT()])
- EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], cs=[$t3])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], expr#4=[123],
expr#5=[null:INTEGER], expr#6=[=($t4, $t5)], expr#7=[IS NULL($t5)],
expr#8=[OR($t6, $t7)], cs=[$t3], $condition=[$t8])
Review comment:
@vlsi I fixed the plan diffs as requested.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388577684
##
File path: core/src/test/resources/sql/sub-query.iq
##
@@ -831,7 +831,7 @@ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t2)],
expr#5=[IS TRUE($t4)], e
EnumerableLimit(fetch=[1])
EnumerableSort(sort0=[$0], dir0=[DESC])
EnumerableAggregate(group=[{0}], c=[COUNT()])
- EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], cs=[$t3])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], expr#4=[123],
expr#5=[null:INTEGER], expr#6=[=($t4, $t5)], expr#7=[IS NULL($t5)],
expr#8=[OR($t6, $t7)], cs=[$t3], $condition=[$t8])
Review comment:
As a human being, you know it is a bad plan, but the cost model thinks it is
a better plan. Shouldn't you blame the cost model?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388576223
##
File path: core/src/test/resources/sql/sub-query.iq
##
@@ -831,7 +831,7 @@ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t2)],
expr#5=[IS TRUE($t4)], e
EnumerableLimit(fetch=[1])
EnumerableSort(sort0=[$0], dir0=[DESC])
EnumerableAggregate(group=[{0}], c=[COUNT()])
- EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], cs=[$t3])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], expr#4=[123],
expr#5=[null:INTEGER], expr#6=[=($t4, $t5)], expr#7=[IS NULL($t5)],
expr#8=[OR($t6, $t7)], cs=[$t3], $condition=[$t8])
Review comment:
How do you know it is skipping some rules? Any evidence?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388575690
##
File path: core/src/test/java/org/apache/calcite/test/LatticeTest.java
##
@@ -677,10 +677,10 @@ private void checkTileAlgorithm(String statisticProvider,
+ "join \"foodmart\".\"time_by_day\" using (\"time_id\")\n"
+ "group by \"the_year\"")
.enableMaterializations(true)
-.explainContains("EnumerableCalc(expr#0=[{inputs}], expr#1=[IS NOT
NULL($t0)], "
-+ "expr#2=[1:BIGINT], expr#3=[0:BIGINT], expr#4=[CASE($t1, $t2,
$t3)], C=[$t4])\n"
-+ " EnumerableAggregate(group=[{0}])\n"
-+ "EnumerableTableScan(table=[[adhoc, m{32, 36}]])")
+.explainContains("EnumerableCalc(expr#0..1=[{inputs}], C=[$t1])\n"
++ " EnumerableAggregate(group=[{0}], C=[COUNT($0)])\n"
++ "EnumerableAggregate(group=[{0}])\n"
Review comment:
I think it is orthogonal. should be done is a separate PR. The issue of cost
model exists before this change.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388571568
##
File path: core/src/test/java/org/apache/calcite/test/LatticeTest.java
##
@@ -677,10 +677,10 @@ private void checkTileAlgorithm(String statisticProvider,
+ "join \"foodmart\".\"time_by_day\" using (\"time_id\")\n"
+ "group by \"the_year\"")
.enableMaterializations(true)
-.explainContains("EnumerableCalc(expr#0=[{inputs}], expr#1=[IS NOT
NULL($t0)], "
-+ "expr#2=[1:BIGINT], expr#3=[0:BIGINT], expr#4=[CASE($t1, $t2,
$t3)], C=[$t4])\n"
-+ " EnumerableAggregate(group=[{0}])\n"
-+ "EnumerableTableScan(table=[[adhoc, m{32, 36}]])")
+.explainContains("EnumerableCalc(expr#0..1=[{inputs}], C=[$t1])\n"
++ " EnumerableAggregate(group=[{0}], C=[COUNT($0)])\n"
++ "EnumerableAggregate(group=[{0}])\n"
Review comment:
It does have the original plan's alternative, but from cost model's
perspective, the new one is a cheaper plan.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388565046
##
File path: core/src/test/resources/sql/sub-query.iq
##
@@ -831,7 +831,7 @@ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t2)],
expr#5=[IS TRUE($t4)], e
EnumerableLimit(fetch=[1])
EnumerableSort(sort0=[$0], dir0=[DESC])
EnumerableAggregate(group=[{0}], c=[COUNT()])
- EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], cs=[$t3])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[false], expr#4=[123],
expr#5=[null:INTEGER], expr#6=[=($t4, $t5)], expr#7=[IS NULL($t5)],
expr#8=[OR($t6, $t7)], cs=[$t3], $condition=[$t8])
Review comment:
The cost model doesn't think it is a degradation:
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387972516
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
In default implementation, there is only 1 phase used. But actually it might
help to have multiple phases to split different kinds of rules.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387961465
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
As a user, you can extend VolcanoPlanner and override the checkCancel method.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387961038
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
I have added a timeout exception and return the current best plan
immediately after timeout. This gives user more ability to customize when to
timeout.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387946486
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Given that the optimizer is not exploring all the join order alternatives
(the catalan number), if the planning can't fit into a finite amount of time,
there is a bug.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387938813
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
It is easy to fix. Don't follow Calcite's way, it is doing wrong.
- Transformation rule matches only logical nodes, not physical nodes.
- Create your rule to match AbstractConverter to convert to your own
physical sort/distribute nodes.
- Customize your own convention to use abstract converter only when both
from and to are physical convention.
If your system is not a federated system, limit your convention to 2: NONE
and Physical one.
But since you already modified the code, you can still add it back in your
own codebase even I removed 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387749721
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
I think you should investigate why your planner takes too much time to
generate a plan.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387723164
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -87,36 +85,11 @@
* according to a dynamic programming algorithm.
*/
public class VolcanoPlanner extends AbstractRelOptPlanner {
- protected static final double COST_IMPROVEMENT = .5;
//~ Instance fields
protected RelSubset root;
- /**
- * If true, the planner keeps applying rules as long as they continue to
- * reduce the cost. If false, the planner terminates as soon as it has found
- * any implementation, no matter how expensive.
- */
- protected boolean ambitious = true;
-
- /**
- * If true, and if {@link #ambitious} is true, the planner waits a finite
- * number of iterations for the cost to improve.
- *
- * The number of iterations K is equal to the number of iterations
- * required to get the first finite plan. After the first finite plan, it
- * continues to fire rules to try to improve it. The planner sets a target
- * cost of the current best cost multiplied by {@link #COST_IMPROVEMENT}. If
- * it does not meet that cost target within K steps, it quits, and uses the
- * current best plan. If it meets the cost, it sets a new, lower target, and
- * has another K iterations to meet it. And so forth.
- *
- * If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Nope. `impatient ` doesn't guarantee to find the best plan, no one ever used
it.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r386187345
##
File path: core/src/test/resources/sql/misc.iq
##
@@ -473,7 +473,7 @@ EnumerableCalc(expr#0..7=[{inputs}], expr#8=[IS
NULL($t5)], expr#9=[IS NULL($t7)
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[true], deptno=[$t0],
$f0=[$t4])
EnumerableTableScan(table=[[hr, depts]])
EnumerableAggregate(group=[{0}], agg#0=[MIN($1)])
- EnumerableCalc(expr#0..3=[{inputs}], expr#4=[90], expr#5=[+($t0, $t4)],
expr#6=[true], $f4=[$t5], $f0=[$t6])
+ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[90], expr#5=[+($t0, $t4)],
expr#6=[true], $f4=[$t5], $f0=[$t6], $condition=[$t6])
EnumerableTableScan(table=[[hr, depts]])
!plan
Review comment:
Not yet specific experiment. But the slow tests in the patch is 23m.
Comparing 32m in master.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
hsyuan commented on a change in pull request #1840: [CALCITE-3753] Remove rule
queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r386183382
##
File path: core/src/test/resources/sql/misc.iq
##
@@ -473,7 +473,7 @@ EnumerableCalc(expr#0..7=[{inputs}], expr#8=[IS
NULL($t5)], expr#9=[IS NULL($t7)
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[true], deptno=[$t0],
$f0=[$t4])
EnumerableTableScan(table=[[hr, depts]])
EnumerableAggregate(group=[{0}], agg#0=[MIN($1)])
- EnumerableCalc(expr#0..3=[{inputs}], expr#4=[90], expr#5=[+($t0, $t4)],
expr#6=[true], $f4=[$t5], $f0=[$t6])
+ EnumerableCalc(expr#0..3=[{inputs}], expr#4=[90], expr#5=[+($t0, $t4)],
expr#6=[true], $f4=[$t5], $f0=[$t6], $condition=[$t6])
EnumerableTableScan(table=[[hr, depts]])
!plan
Review comment:
Yes, since the total cost is the same.
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:
us...@infra.apache.org
With regards,
Apache Git Services
