[GitHub] [calcite] hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r405929414
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
When we have a query `select * from foo limit 5`, there is still a sort
operator in the plan, but it doesn't do any sort work.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r405928980
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
I know it is confusing, but it is the design (Sort operator mixes both sort
and limit operator) that leads to the confusion. We should have a separate
LIMIT operator, SORT operator should not have limit and offset.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r405928280
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
No, the collation is required by LIMIT.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r405912405
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
Basically it is a limit operator. The parent of LIMIT doesn't require
anything from 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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r404541078
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
No
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r404539667
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
Yes, order by is an enforcer. Because the top required trait is something
like EnumerableConvention with a collation.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r404428336
##
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##
@@ -157,6 +157,11 @@ public RelNode accept(RexShuttle shuttle) {
return copy(traitSet, getInput(), collation, offset, fetch);
}
+ @Override public boolean isEnforcer() {
+return offset == null && fetch == null
Review comment:
Good question, the answer is no. The sort in your example is created no
matter it satisfies parent's required trait or not. So this is not an enforcer.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r401312442
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -160,107 +162,106 @@ void obliterateRelNode(RelNode rel) {
public RelSubset add(RelNode rel) {
assert equivalentSet == null : "adding to a dead set";
final RelTraitSet traitSet = rel.getTraitSet().simplify();
-final RelSubset subset = getOrCreateSubset(rel.getCluster(), traitSet);
+final RelSubset subset = getOrCreateSubset(
+rel.getCluster(), traitSet, rel.isEnforcer());
subset.add(rel);
return subset;
}
+ /**
+ * If the subset is required, convert derived subsets to this subset.
Review comment:
I think it is good to call `isRequired`. Because it is the state of the
RelSubset.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r400967195
##
File path: core/src/main/java/org/apache/calcite/plan/Convention.java
##
@@ -44,7 +44,9 @@
* @param toConvention Desired convention to convert to
* @return Whether we should convert from this convention to toConvention
*/
- boolean canConvertConvention(Convention toConvention);
+ default boolean canConvertConvention(Convention toConvention) {
+return false;
Review comment:
[CALCITE-1148](https://github.com/apache/calcite/pull/210) introduced it. It
is just a default value of the interface, no behavior changes.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r400966613
##
File path: core/src/main/java/org/apache/calcite/plan/Convention.java
##
@@ -59,8 +61,10 @@
* @param toTraits Target traits
* @return Whether we should add converters
*/
- boolean useAbstractConvertersForConversion(RelTraitSet fromTraits,
- RelTraitSet toTraits);
+ default boolean useAbstractConvertersForConversion(RelTraitSet fromTraits,
+ RelTraitSet toTraits) {
+return true;
Review comment:
[CALCITE-1148](https://github.com/apache/calcite/pull/210) introduced it,
which is a walk-around for the inefficiency. Now we can turn it on by default.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r400956153
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -160,107 +162,106 @@ void obliterateRelNode(RelNode rel) {
public RelSubset add(RelNode rel) {
assert equivalentSet == null : "adding to a dead set";
final RelTraitSet traitSet = rel.getTraitSet().simplify();
-final RelSubset subset = getOrCreateSubset(rel.getCluster(), traitSet);
+final RelSubset subset = getOrCreateSubset(
+rel.getCluster(), traitSet, rel.isEnforcer());
subset.add(rel);
return subset;
}
+ /**
+ * If the subset is required, convert derived subsets to this subset.
Review comment:
It is ok to say that. Because we will call `relsubset. isRequired()` and
`relsubset.isDerived()` on RelSubSet.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset URL: https://github.com/apache/calcite/pull/1860#discussion_r400953759 ## File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java ## @@ -98,10 +100,13 @@ long timestamp; /** - * Flag indicating whether this RelSubset's importance was artificially - * boosted. + * Physical property state of current subset + * 0: logical operators, NONE convention is neither DERIVED nor REQUIRED + * 1: traitSet DERIVED from child operators or itself + * 2: traitSet REQUIRED from parent operators + * 3: both DERIVED and REQUIRED */ - boolean boosted; + int state = 0; Review comment: Fixed, thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r398088521
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -337,10 +339,20 @@ void mergeWith(
// merge subsets
for (RelSubset otherSubset : otherSet.subsets) {
- RelSubset subset =
- getOrCreateSubset(
- otherSubset.getCluster(),
- otherSubset.getTraitSet());
+ RelSubset subset = null;
+ RelTraitSet otherTraits = otherSubset.getTraitSet();
+
+ // if it is logical or derived physical traitSet
+ if (otherSubset.state == 0 || otherSubset.isDerived()) {
+subset = getOrCreateSubset(cluster, otherTraits, false);
+ }
+
+ // it may be required only, or both derived and required,
+ // in which case, register again.
+ if (otherSubset.isRequired()) {
Review comment:
fixed, thanks.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r398088611
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -160,107 +162,106 @@ void obliterateRelNode(RelNode rel) {
public RelSubset add(RelNode rel) {
assert equivalentSet == null : "adding to a dead set";
final RelTraitSet traitSet = rel.getTraitSet().simplify();
-final RelSubset subset = getOrCreateSubset(rel.getCluster(), traitSet);
+final RelSubset subset = getOrCreateSubset(
+rel.getCluster(), traitSet, rel.isEnforcer());
subset.add(rel);
return subset;
}
+ /**
+ * If the subset is required, convert derived subsets to this subset.
+ * Otherwise, convert this subset to required subsets in this RelSet.
+ * The subset can be both required and derived.
+ */
private void addAbstractConverters(
- VolcanoPlanner planner, RelOptCluster cluster, RelSubset subset, boolean
subsetToOthers) {
-// Converters from newly introduced subset to all the remaining one (vice
versa), only if
-// we can convert. No point adding converters if it is not possible.
-for (RelSubset other : subsets) {
+ RelOptCluster cluster, RelSubset subset, boolean required) {
+List others = subsets.stream().filter(
+n -> required ? n.isDerived() : n.isRequired())
+.collect(Collectors.toList());
+for (RelSubset other : others) {
assert other.getTraitSet().size() == subset.getTraitSet().size();
+ RelSubset from = subset;
+ RelSubset to = other;
+
+ if (required) {
+from = other;
+to = subset;
+ }
- if ((other == subset)
- || (subsetToOthers
- && !subset.getConvention().useAbstractConvertersForConversion(
- subset.getTraitSet(), other.getTraitSet()))
- || (!subsetToOthers
- && !other.getConvention().useAbstractConvertersForConversion(
- other.getTraitSet(), subset.getTraitSet( {
+ if (from == to || !from.getConvention()
+ .useAbstractConvertersForConversion(
+ from.getTraitSet(), to.getTraitSet())) {
continue;
}
final ImmutableList difference =
- subset.getTraitSet().difference(other.getTraitSet());
+ to.getTraitSet().difference(from.getTraitSet());
- boolean addAbstractConverter = true;
- int numTraitNeedConvert = 0;
-
- for (RelTrait curOtherTrait : difference) {
-RelTraitDef traitDef = curOtherTrait.getTraitDef();
-RelTrait curRelTrait = subset.getTraitSet().getTrait(traitDef);
-
-if (curRelTrait == null) {
- addAbstractConverter = false;
- break;
-}
+ boolean needsConverter = false;
-assert curRelTrait.getTraitDef() == traitDef;
-
-boolean canConvert = false;
-boolean needConvert = false;
-if (subsetToOthers) {
- // We can convert from subset to other. So, add converter with
subset as child and
- // traitset as the other's traitset.
- canConvert = traitDef.canConvert(
- cluster.getPlanner(), curRelTrait, curOtherTrait, subset);
- needConvert = !curRelTrait.satisfies(curOtherTrait);
-} else {
- // We can convert from others to subset.
- canConvert = traitDef.canConvert(
- cluster.getPlanner(), curOtherTrait, curRelTrait, other);
- needConvert = !curOtherTrait.satisfies(curRelTrait);
-}
+ for (RelTrait fromTrait : difference) {
+RelTraitDef traitDef = fromTrait.getTraitDef();
+RelTrait toTrait = to.getTraitSet().getTrait(traitDef);
-if (!canConvert) {
- addAbstractConverter = false;
+if (toTrait == null || !traitDef.canConvert(
+cluster.getPlanner(), fromTrait, toTrait, from)) {
+ needsConverter = false;
break;
}
-if (needConvert) {
- numTraitNeedConvert++;
+if (!fromTrait.satisfies(toTrait)) {
+ needsConverter = true;
}
}
- if (addAbstractConverter && numTraitNeedConvert > 0) {
-if (subsetToOthers) {
- final AbstractConverter converter =
- new AbstractConverter(cluster, subset, null,
other.getTraitSet());
- planner.register(converter, other);
-} else {
- final AbstractConverter converter =
- new AbstractConverter(cluster, other, null,
subset.getTraitSet());
- planner.register(converter, subset);
-}
+ if (needsConverter) {
+final AbstractConverter converter =
+new AbstractConverter(cluster, from, null, to.getTraitSet());
+cluster.getPlanner().register(converter, to);
}
}
}
+ RelSubset
[GitHub] [calcite] hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395432588
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
If you turn on abstract converter in master branch, it generates the same
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395432588
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
If you turn abstract converter in master branch, it generates the same 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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395431348
##
File path: core/src/main/java/org/apache/calcite/rel/RelNode.java
##
@@ -407,6 +407,17 @@ RelNode copy(
*/
void register(RelOptPlanner planner);
+ /**
+ * Indicates whether it is an enforcer operator, e.g. PhysicalSort,
+ * PhysicalHashDistribute, etc. As an enforcer, the operator must be
+ * created only when required traitSet is not satisfied by its input.
+ *
+ * @return Whether it is an enforcer operator
+ */
+ default boolean isEnforcer() {
+return false;
Review comment:
we can remove AbstractRelNode. isEnforcer
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395431225
##
File path:
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableConvention.java
##
@@ -62,6 +62,6 @@ public boolean canConvertConvention(Convention toConvention)
{
public boolean useAbstractConvertersForConversion(RelTraitSet fromTraits,
RelTraitSet toTraits) {
-return false;
+return !fromTraits.containsIfApplicable(Convention.NONE);
}
Review comment:
It could be. I wish so.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset URL: https://github.com/apache/calcite/pull/1860#discussion_r395228189 ## File path: core/src/main/java/org/apache/calcite/rel/RelNode.java ## @@ -407,6 +407,15 @@ RelNode copy( */ void register(RelOptPlanner planner); + /** + * Indicates whether it is an enforcer operator, e.g. PhysicalSort, + * PhysicalHashDistribute, etc. As an enforcer, the operator must be + * created only when required traitSet is not satisfied by its input. + * + * @return Whether it is an enforcer operator + */ + boolean isEnforcer(); Review comment: Yes, will do. 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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395217146
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
@vlsi Correct.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r395169171
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
The cost model is orthogonal with 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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r394761318
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
The traitset can be both required and derived.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r394761404
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
I think so.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r394761359
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSubset.java
##
@@ -144,6 +141,22 @@ private void computeBestCost(RelOptPlanner planner) {
}
}
+ public void setDerived() {
+state |= DERIVED;
+ }
+
+ public void setRequired() {
+state |= REQUIRED;
+ }
+
+ public boolean isDerived() {
+return (state & DERIVED) == DERIVED;
+ }
+
+ public boolean isRequired() {
Review comment:
The traitset can be both required and derived.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r394761318
##
File path: core/src/test/java/org/apache/calcite/test/StreamTest.java
##
@@ -286,15 +286,17 @@ private static String schemaFor(String name, Class clazz
+ " LogicalTableScan(table=[[STREAM_JOINS, PRODUCTS]])\n")
.explainContains(""
+ "EnumerableCalc(expr#0..6=[{inputs}], proj#0..1=[{exprs}],
SUPPLIERID=[$t6])\n"
-+ " EnumerableHashJoin(condition=[=($4, $5)], joinType=[inner])\n"
-+ "EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
-+ " EnumerableInterpreter\n"
-+ "BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
-+ "EnumerableTableScan(table=[[STREAM_JOINS, PRODUCTS]])")
++ " EnumerableMergeJoin(condition=[=($4, $5)],
joinType=[inner])\n"
++ "EnumerableSort(sort0=[$4], dir0=[ASC])\n"
++ " EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[CAST($t2):VARCHAR(32) NOT NULL], proj#0..4=[{exprs}])\n"
++ "EnumerableInterpreter\n"
++ " BindableTableScan(table=[[STREAM_JOINS, ORDERS,
(STREAM)]])\n"
Review comment:
The traitset can be both required and derived.
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
