[GitHub] [calcite] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r405927959
##
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:
The collation the required by the root, same as the ORDER BY case.
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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r405899756
##
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 don't understand. In a lot of systems, the only difference is an extra
limit operator on top of the sort. I don't get why it doesn't have required
trait.
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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r404540805
##
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:
And ORDER BY ... LIMIT doesn't have such required trait?
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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r404531494
##
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:
What if just ORDER BY ...? So the Sort operator all the sudden become an
enforcer when LIMIT is not presented?
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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r404422213
##
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:
This is the definition from you - "As an enforcer, the operator must be
created only when required traitSet is not satisfied by its input." So it
sounds to me that only the caller (a RelOptRule or the framework) who creates
the RelNode would know if the RelNode is a converter or not. If this
information is not passed into the RelNode, how can we just derive this
information from RelNode itself? In this particular case, if Sort is used in
ORDER BY ... LIMIT ... scenario, it's still an enforcer. 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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r400561640
##
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:
I also feel it's confusing to have REQUIRE/DERIVED status 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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r400584476
##
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:
why offset and fetch have to be null? I feel that isEnforcer should be
something passed down when the RelNode is created. It's hard to tell by itself
that if an operator is served as 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] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r400556733
##
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:
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r400556857
##
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:
Why do we want to change the base method?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xndai commented on a change in pull request #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
xndai 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_r400560188
##
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's a little confusing to say "required subset" or "derived subset". I
think what you mean is required/derived trait from subset.
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
