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/02/29 00:02:38 UTC
[GitHub] [calcite] hsyuan opened a new pull request #1840: [CALCITE-3753]
Remove rule queue importance
hsyuan opened a new pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840
JIRA: https://issues.apache.org/jira/browse/CALCITE-3753
I will update test cases later.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389437622
##########
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:
Okey, thanks for the explanation.
----------------------------------------------------------------
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] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387924742
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
We've investigated it and looks like it is very similar to the [abstract converters problem](https://issues.apache.org/jira/browse/CALCITE-2970). `impatient` flag was quite helpful for us in some cases.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387935235
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Can we rename `impatient` flag to [`useSimulatedAnnealing`](https://en.wikipedia.org/wiki/Simulated_annealing) ?
It would sound sophisticated enough to keep it from removing :)
Jokes aside, I think the planning cannot be always fit into a finite amount of time, so it makes sense to have a predefined timeout for the planner. That is it could give up and just return the best it has so far.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389206283
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateRemoveRule.java
##########
@@ -123,6 +123,7 @@ public void onMatch(RelOptRuleCall call) {
// aggregate functions, add a project for the same effect.
relBuilder.project(relBuilder.fields(aggregate.getGroupSet()));
}
+ call.getPlanner().setImportance(aggregate, 0d);
call.transformTo(relBuilder.build());
Review comment:
Why we need 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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389206452
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/FilterProjectTransposeRule.java
##########
@@ -45,17 +47,13 @@
/** The default instance of
* {@link org.apache.calcite.rel.rules.FilterProjectTransposeRule}.
*
- * <p>It matches any kind of {@link org.apache.calcite.rel.core.Join} or
- * {@link org.apache.calcite.rel.core.Filter}, and generates the same kind of
- * Join and Filter.
- *
* <p>It does not allow a Filter to be pushed past the Project if
* {@link RexUtil#containsCorrelation there is a correlation condition})
* anywhere in the Filter, since in some cases it can prevent a
* {@link org.apache.calcite.rel.core.Correlate} from being de-correlated.
*/
public static final FilterProjectTransposeRule INSTANCE =
- new FilterProjectTransposeRule(Filter.class, Project.class, true, true,
+ new FilterProjectTransposeRule(LogicalFilter.class, LogicalProject.class, true, true,
Review comment:
A breaking 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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389206730
##########
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:
Why another sort?
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389205194
##########
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:
Why we need a 0.9 factor?
----------------------------------------------------------------
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 issue #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-593202058
> Plan diffs are more than what i expected, BTW, what is the diff tool, it looks pretty good ~
It is diffchecker.
----------------------------------------------------------------
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 issue #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-596050901
@zabetak Thanks for reminding. Will update 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan closed pull request #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387557976
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Is there any way now to stop the planner search without exhaustive search space exploration? `impatient` flag was pretty useful for 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389569220
##########
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:
We can not force every limit test to have an order by, can we have a more nice solution to promotion the stability, i.e. if a make this change into Flink, there would be some test fails but i would not expect to modify the queries.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387953477
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
There is always a trade-off between planning time and plan quality. If I have a very complex query I don't want have the best plan ever with a very long planning time. I want some 'acceptable' plan obtained in 'acceptable' time.
It is a good practice to have an optimization timeout, especially in production systems. See optimizer timeout for MS SQL Server.
`impatient` flag was the kind of this timeout. I'm not insist that we should keep exactly this flag, but throwing it away without an alternative is not a good idea, IMHO.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388218456
##########
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:
This looks like a plan degradation.
For instance `expr#4=[123], expr#5=[null:INTEGER], expr#6=[=($t4, $t5)]` is the same as `null:BOOLEAN`.
Do you know the reason for this plan 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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389329225
##########
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:
Calcite executes the query with single thread, so theoretically, there shouldn't be any un-stability.
----------------------------------------------------------------
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 issue #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-593072264
All the plan diffs in this patch are either with same or less cost.
----------------------------------------------------------------
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] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387945975
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Yes, there are many ways to fix this problem. But anyway, I agree with Vladimir that we need an ability to restrict the planning time. It is a good point.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388570453
##########
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:
Let me put it in another way: you change the optimizer, and now it favours bad plans.
What the optimizer now does it introduces a dummy always_true filter, and it thinks the filter would reduce the number of rows and so on. It does not look like a well-behaving optimizer :-/
Even though the change reduces `slow test` execution, that reduction might be the result of "skipping some rules" rather than removing importance.
So currently it looks like some rules do not fire which result in a noticeable amount of useless predicates floating around.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388573407
##########
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:
Who will fix the costing model then?
I think it is unfair to merge a change that is not really compatible with the costing model.
If the change to optimizer requires adjustments to the costing model, then could you please do that in a single commit, so we see the net changes for both plans and the response times?
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r386182933
##########
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:
Is this plan change 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] hsyuan commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xndai commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387985578
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
We should still keep planning phase. In classic volcano model, there are transformation, implementation and optimization phases. The planner can do specific things for different phases. It's not used now, but we could probably extend it in the future.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388579721
##########
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:
OK. You propose a change. It results in generating bad plans, thus it introduces a technical regression. There's a technical justification for -1.
I know cost model has awful lot of inconsistencies. However, it turns out that all those 100 tiny inconsistencies cancel each other, and Calcite manages to produce "sane" plans.
Now you fix one or two such defects (which has good merit), however, the net result becomes that there are 98 inconsistencies in the cost model which **no longer** cancel each other.
Calcite purpose is optimizer, and it is really sad to introduce regressions to the optimizer.
----------------------------------------------------------------
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 #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-593199798
> Here is one example:
> 
Plan diffs are more than what i expected, BTW, what is the diff tool, it looks pretty good ~
----------------------------------------------------------------
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] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387557976
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Is there any way now to stop the planner search without search space exhaustive exploration? `impatient` flag was pretty useful for 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:
users@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
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r386185952
##########
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:
I think optimizer latency can be benefited a lot from this patch. Any experiment?
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388573407
##########
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:
Who will fix the costing model then?
I think it is unfair to merge a change that is not really compatible with the costing model.
If the change to optimizer requires adjustments to the costing model, then could you please do that in a single PR, so we see the net changes for both plans and the response times?
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389436659
##########
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:
But with same query and same rule sets, the rules should be fired in same sequence 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] hsyuan commented on issue #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-593073365
Here is one example:
----------------------------------------------------------------
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] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387967624
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
Ok, it might help us.
Off-topic question: why do we need several `VolcanoPlannerPhase` when only the one of them is used? May be we can throw it away along with the queue 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] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389205888
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
I agree Haidai, we should have an alternative, or I would see it as a regression, we never have a perfect planner. Instead of running into timeout, a motive control flag is more acceptable and user friendly.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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.
- *
- * <p>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.
- *
- * <p>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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389205888
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
I agree with @rkondakov, we should have an alternative, or I would see it as a regression, we never have a perfect planner. Instead of running into timeout, a motive control flag is more acceptable and user friendly.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388570453
##########
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:
Let me put it in another way: you change the optimizer, and now it favours bad plans.
What it does it introduces a dummy always_true filter, and it thinks the filter would reduce the number of rows and so on. It does not look like a well-behaving optimizer :-/
Even though the change reduces `slow test` execution, that reduction might be the result of "skipping some rules" rather than removing importance.
So currently it looks like some rules do not fire which result in a noticeable amount of useless predicates floating around.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1840: [CALCITE-3753] Remove
rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-596266573
Thanks everyone for reviewing. I will merge this PR in 24 hours.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r388213305
##########
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:
This looks like a plan degradation, doesn't 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389206812
##########
File path: geode/src/main/java/org/apache/calcite/adapter/geode/rel/GeodeSort.java
##########
@@ -58,7 +58,7 @@
if (fetch != null) {
return cost.multiplyBy(0.05);
} else {
- return cost;
+ return cost.multiplyBy(0.9);
Review comment:
Why?
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rkondakov commented on a change in pull request #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r387738554
##########
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.
- *
- * <p>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.
- *
- * <p>If false, the planner continues to fire rules until the rule queue is
- * empty.
- */
- protected boolean impatient = false;
Review comment:
You are right, 'impatient' flag doesn't help to find the best plan. But it helps to interrupt the search if it takes too much time and you are ok to go ahead with a sub optimal plan. We use it for this purpose.
----------------------------------------------------------------
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 edited a comment on issue #1840: [CALCITE-3753]
Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
hsyuan edited a comment on issue #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#issuecomment-593072264
All the plan diffs in this patch are with same cost.
----------------------------------------------------------------
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 #1840:
[CALCITE-3753] Remove rule queue importance
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1840: [CALCITE-3753] Remove rule queue importance
URL: https://github.com/apache/calcite/pull/1840#discussion_r389329107
##########
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:
It is weird to tweak the cost to select a specific Convention and why should the `JDBC` convention should be cheaper ?
----------------------------------------------------------------
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