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/05/20 06:54:19 UTC
[GitHub] [calcite] amaliujia opened a new pull request #1985: [WIP][CALCITE-4011] Implement trait propagation for EnumerableProject…
amaliujia opened a new pull request #1985:
URL: https://github.com/apache/calcite/pull/1985
… and EnumerableFilter
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428399349
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -65,92 +70,184 @@
* </ol>
*/
class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
+ // Remove to Keep deterministic join order
+ private static final List<RelOptRule> NON_DETERMINISTIC_JOIN_ORDER_RULES =
+ ImmutableList.of(
+ JoinCommuteRule.INSTANCE,
+ JoinPushThroughJoinRule.LEFT,
+ JoinPushThroughJoinRule.RIGHT
+ );
+
+ // Ordering enforcement should not be done by rules
+ private static final List<RelOptRule> SORT_PROJECT_TRANSPOSE_RULES =
+ ImmutableList.of(
+ SortProjectTransposeRule.INSTANCE,
+ ProjectSortTransposeRule.INSTANCE
+ );
+
+ // Remove to always use merge join
+ private static final List<RelOptRule> NON_MERGE_JOIN_RULES =
+ ImmutableList.of(
+ EnumerableRules.ENUMERABLE_JOIN_RULE,
+ EnumerableRules.ENUMERABLE_CALC_RULE
+ );
@Test void testSortAgg() {
final String sql = "select mgr, count(*) from sales.emp\n"
+ "group by mgr order by mgr desc nulls last limit 5";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE).check();
}
@Test void testSortAggPartialKey() {
final String sql = "select mgr,deptno,comm,count(*) from sales.emp\n"
+ "group by mgr,deptno,comm\n"
+ "order by comm desc nulls last, deptno nulls first";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testSortMergeJoin() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by r.job desc nulls last, r.ename nulls first";
- sql(sql).check();
+ Query.create(sql).removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES).check();
}
@Test void testSortMergeJoinRight() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by s.job desc nulls last, s.ename nulls first";
- sql(sql).check();
+ Query.create(sql).removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES).check();
}
@Test void testMergeJoinDeriveLeft1() {
final String sql = "select * from\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveLeft2() {
final String sql = "select * from\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight1() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight2() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
Review comment:
I think they should be removed by default
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
should be assert?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
Review comment:
use `required.getCollation()`
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,39 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation =
+ getCluster().traitSet().canonize(RexUtil.apply(map, collation));
Review comment:
Use `collation.apply(map)`.
No need to canonize, it is already canonized when you get it.
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
##########
@@ -78,4 +78,6 @@
boolean typeCoercion();
/** @see CalciteConnectionProperty#LENIENT_OPERATOR_LOOKUP */
boolean lenientOperatorLookup();
+ /** @see CalciteConnectionProperty#VOLCANO_TOPDOWN_OPT */
+ boolean enableVolcanoTopDownOptimization();
Review comment:
Let's use `boolean topDownOpt()` to keep consistent with other method signatures.
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
##########
@@ -159,7 +159,10 @@
/** Whether to make create implicit functions if functions do not exist
* in the operator table, default false. */
- LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false);
+ LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false),
+
+ /** Whether to enable top-down optimization in Volcano planner. */
+ VOLCANO_TOPDOWN_OPT("enableVolcanoTopDownOptimization", Type.BOOLEAN, false, false);
Review comment:
How about remove `VOLCANO_`? volcano should be kept as inside term only.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432111811
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
Added the missing CAST part.
It was due to how tests are written. E.g. the cast leads to non-monotonic behaves the same of a normal RexCall. Thus even CAST misses, nothing was broken.
I used debugger to confirm now that CAST test cases hit the correct code path.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635765563
removed assert `OptimizeTask.java:231`.
Also when there is no more comment, I will squash all comments into one with a detailed commit message about 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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432171885
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +103,86 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ int maxField = Math.max(getInput().getRowType().getFieldCount(), getProjects().size());
+ Mappings.TargetMapping mapping = Mappings
+ .create(MappingType.FUNCTION, maxField, maxField);
+ for (Ord<RexNode> node : Ord.zip(getProjects())) {
+ if (node.e instanceof RexInputRef) {
+ mapping.set(((RexInputRef) node.e).getIndex(), node.i);
+ } else if (node.e.isA(SqlKind.CAST)) {
+ final RexNode operand = ((RexCall) node.e).getOperands().get(0);
+ if (operand instanceof RexInputRef) {
+ mapping.set(((RexInputRef) operand).getIndex(), node.i);
+ }
+ }
+ }
+
+ List<RelFieldCollation> collationFieldsToDerive = new ArrayList<>();
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (isCollationOnTrivialExpr(mapping, rc)) {
Review comment:
This needs a second thought for `deriveTraits`
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +103,86 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ int maxField = Math.max(getInput().getRowType().getFieldCount(), getProjects().size());
+ Mappings.TargetMapping mapping = Mappings
+ .create(MappingType.FUNCTION, maxField, maxField);
+ for (Ord<RexNode> node : Ord.zip(getProjects())) {
+ if (node.e instanceof RexInputRef) {
+ mapping.set(((RexInputRef) node.e).getIndex(), node.i);
+ } else if (node.e.isA(SqlKind.CAST)) {
+ final RexNode operand = ((RexCall) node.e).getOperands().get(0);
+ if (operand instanceof RexInputRef) {
+ mapping.set(((RexInputRef) operand).getIndex(), node.i);
+ }
+ }
+ }
+
+ List<RelFieldCollation> collationFieldsToDerive = new ArrayList<>();
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (isCollationOnTrivialExpr(mapping, rc)) {
+ collationFieldsToDerive.add(rc);
+ } else {
+ break;
+ }
+ }
+
+ if (collationFieldsToDerive.size() > 0) {
+ final RelCollation newCollation = RelCollations.of(collationFieldsToDerive).apply(mapping);
+ return Pair.of(childTraits.replace(newCollation), ImmutableList.of(childTraits));
+ } else {
+ return null;
+ }
+ }
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ private boolean isCollationOnTrivialExpr(
+ Mappings.TargetMapping map, RelFieldCollation fc) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return false;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
Review comment:
Try this query:
```sql
select comm, deptno, slacker from
(select * from sales.emp order by COMM, DEPTNO, SLACKER limit 10) t
order by comm, slacker;
```
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431907434
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
If you don't want to support cast in `derive`, then the check `isCollationOnTrivialExpr` is not need anymore.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635712992
Also try to turn on top-down mode globally and run the tests, make sure there is no exception or assertion error.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431071501
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
Correct, this is expected.
Can you add some comments to state the purpose of these tests. Sorry for not giving a good example in my code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429586103
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Good point. Let me check this a bit.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429570352
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
My current understanding is, we want to make sure Top-down optimization does not affect existing implementation.
Then, not sure if you are aware of that, the value in `saffron.properties` always overwrite `CalciteSystemProperty.TOPDOWN_OPT`, thus `CalciteSystemProperty.TOPDOWN_OPT` is always true.
Thus, top-down optimizations are enabled by default in many tests. And the only way to update it is calling `planner.setTopDownOpt`.
This line is trying to disable top-down opt by default for JDBC code path (e.g. JDBC tests). Without this linke, top-down opt is enabled and ~40 tests are failing due to trait propagation implementation in this PR.
So do I understand correctly that top-down opt should be implemented right now in a relative isolated environment? Can we set the value in `saffron.properties` as false to turn off top-down opt by default? Is there better suggestions?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432253642
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +103,86 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ int maxField = Math.max(getInput().getRowType().getFieldCount(), getProjects().size());
+ Mappings.TargetMapping mapping = Mappings
+ .create(MappingType.FUNCTION, maxField, maxField);
+ for (Ord<RexNode> node : Ord.zip(getProjects())) {
+ if (node.e instanceof RexInputRef) {
+ mapping.set(((RexInputRef) node.e).getIndex(), node.i);
+ } else if (node.e.isA(SqlKind.CAST)) {
+ final RexNode operand = ((RexCall) node.e).getOperands().get(0);
+ if (operand instanceof RexInputRef) {
+ mapping.set(((RexInputRef) operand).getIndex(), node.i);
+ }
+ }
+ }
+
+ List<RelFieldCollation> collationFieldsToDerive = new ArrayList<>();
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (isCollationOnTrivialExpr(mapping, rc)) {
+ collationFieldsToDerive.add(rc);
+ } else {
+ break;
+ }
+ }
+
+ if (collationFieldsToDerive.size() > 0) {
+ final RelCollation newCollation = RelCollations.of(collationFieldsToDerive).apply(mapping);
+ return Pair.of(childTraits.replace(newCollation), ImmutableList.of(childTraits));
+ } else {
+ return null;
+ }
+ }
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ private boolean isCollationOnTrivialExpr(
+ Mappings.TargetMapping map, RelFieldCollation fc) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return false;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
Review comment:
Yes. Good point.
Need to add
```
if (passdown) {
node = getProjects().get(fc.getFieldIndex());
} else {
node = getProjects().get(map.getTarget(fc.getFieldIndex()));
}
```
The old tests passes because I actually used the first few columns in test cases, thus derive traits wasn't affected.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432256369
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +73,23 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ if (required.getCollation() == null) {
Review comment:
In fact we still need `collation == null`. Turns out that after turning on globally top-down opt, there still NPE hitting this `if` because collation is null.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia edited a comment on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635765563
removed assert at `OptimizeTask.java:231`.
Also when there is no more comment, I will squash all comments into one with a detailed commit message about 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428433821
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
Ah maybe not.
There are several test cases that passed into null or empty fields collation. Had to return null, otherwise tests will fail.
However, I don't know if null or empty fields collation is correct (need investigate where does that come from).
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432202115
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +73,23 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ if (required.getCollation() == null) {
Review comment:
Indeed, the collation check is needed. Let's add this check back
```
required.getCollation() == RelCollations.EMPTY
```
Because it is possible that only convention is different.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431473791
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -128,36 +132,42 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
final Mappings.TargetMapping map =
RelOptUtil.permutationIgnoreCast(
getProjects(), getInput().getRowType());
- if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
- return null;
+
+ List<RelFieldCollation> collationFieldsToDerive = new ArrayList<>();
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (isCollationOnTrivialExpr(map, rc)) {
+ collationFieldsToDerive.add(rc);
+ }
Review comment:
Let's see this query's plan:
```sql
select a, b*-1 as b, c from (select * from foo order by a, b, c limit 10) order by a, c;
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431484135
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Why?
The top project does not need to enforce a sort because inner sort has provide the needed ordering.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431605210
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
Will make a change (which is easy), just add a break after encountering first non-trivial expr.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429056259
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Can you use the query I provided as the test case for Filter to replace the one you added?
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429050900
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,38 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
Review comment:
This can use getCollation too.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
I remembered in `EnumerableConvention.enforce()` it has the same piece of code. :)
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -65,92 +70,177 @@
* </ol>
*/
class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
+ // Ordering enforcement should not be done by rules
+ private static final List<RelOptRule> SORT_PROJECT_TRANSPOSE_RULES =
+ ImmutableList.of(
+ SortProjectTransposeRule.INSTANCE,
+ ProjectSortTransposeRule.INSTANCE
+ );
+
+ // Remove to always use merge join
+ private static final List<RelOptRule> NON_MERGE_JOIN_RULES =
+ ImmutableList.of(
+ EnumerableRules.ENUMERABLE_JOIN_RULE,
+ EnumerableRules.ENUMERABLE_CALC_RULE
Review comment:
Sorry, maybe ENUMERABLE_CALC_RULE is not needed. I found that it is not registered by default.
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -65,92 +70,177 @@
* </ol>
*/
class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
Review comment:
These 2 might be better in the default rule setting of `Query`, always use sorted agg in our tests.
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -65,92 +70,177 @@
* </ol>
*/
class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
+ // Ordering enforcement should not be done by rules
+ private static final List<RelOptRule> SORT_PROJECT_TRANSPOSE_RULES =
+ ImmutableList.of(
+ SortProjectTransposeRule.INSTANCE,
+ ProjectSortTransposeRule.INSTANCE
+ );
+
+ // Remove to always use merge join
+ private static final List<RelOptRule> NON_MERGE_JOIN_RULES =
+ ImmutableList.of(
+ EnumerableRules.ENUMERABLE_JOIN_RULE,
+ EnumerableRules.ENUMERABLE_CALC_RULE
+ );
@Test void testSortAgg() {
final String sql = "select mgr, count(*) from sales.emp\n"
+ "group by mgr order by mgr desc nulls last limit 5";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE).check();
}
@Test void testSortAggPartialKey() {
final String sql = "select mgr,deptno,comm,count(*) from sales.emp\n"
+ "group by mgr,deptno,comm\n"
+ "order by comm desc nulls last, deptno nulls first";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testSortMergeJoin() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by r.job desc nulls last, r.ename nulls first";
- sql(sql).check();
+ Query.create(sql).check();
}
@Test void testSortMergeJoinRight() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by s.job desc nulls last, s.ename nulls first";
- sql(sql).check();
+ Query.create(sql).check();
}
@Test void testMergeJoinDeriveLeft1() {
final String sql = "select * from\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveLeft2() {
final String sql = "select * from\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight1() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight2() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
+ }
+
+ @Test void testSortProject() {
+ final String sql = "select mgr from sales.emp order by mgr desc nulls last";
+ Query.create(sql)
+ .removeRules(SORT_PROJECT_TRANSPOSE_RULES)
Review comment:
The `Query` should remove this rule by default. We don't need it in any case.
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
Hmm... This will always override of config value from CalciteSystemProperty when CalcitePrepareImpl.java is used. We need to think about it.
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
##########
@@ -199,4 +199,8 @@ public boolean lenientOperatorLookup() {
return CalciteConnectionProperty.LENIENT_OPERATOR_LOOKUP.wrap(properties)
.getBoolean();
}
+
+ public boolean topDownOpt() {
Review comment:
Can you add a comment that this value will override the value from `saffron.properties`?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580328
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
Sounds good.
I set top-down opt as false in `saffron.properties`. Then made `CalciteConnectionProperty#TOPDOWN_OPT`'s default value is set by `CalciteSystemProperty.TOPDOWN_OPT.value()`, and I still keep this line code.
So it will achieve:
1. saffron.properties still can control top-down opt globally.
2. Leave a room for people who wants to turn off top-down opt through Calcite connections.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429571862
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
##########
@@ -159,7 +159,10 @@
/** Whether to make create implicit functions if functions do not exist
* in the operator table, default false. */
- LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false);
+ LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false),
+
+ /** Whether to enable top-down optimization in Volcano planner. */
+ TOPDOWN_OPT("topDownOpt", Type.BOOLEAN, false, false);
Review comment:
Do you think it is OK to use the CalciteSystemProperty.TOPDOWN_OPT.value() as the default value?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428433821
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
Ah maybe not.
There are several test cases that passed into null or empty fields collation. If not return null tests will fail.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429585412
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
What we want to test is not whether EnumerableSort is enforced or not, but the collation is passed through the Filter or not.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431969020
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
Let me dig into this part of code and see what is going on.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428361297
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
I setup debugger and confirmed that there was a `EnumerableSort` enforced for `EnumerableFilter`, however, I wasn't sure why planner always think that this plan is the cheapest. In fact, I was hoping to see the following plan:
```
EnumerableProject(MGR=[$3])
EnumerableFilter(condition=[>($7, 10)])
EnumerableSort(sort0=[$3], dir0=[DESC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-633737408
Also implemented traits derivation for both Project and Filter.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429571399
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
Sorry for not being clear. `CalciteSystemProperty. TOPDOWN_OPT` reads data from `saffron.properties`, so it is intended to be globally true, because I wanted to make sure it passed **all** the tests. But now since we are going to add trait propagation for all the operators, and in next release, we will add top-down rule apply, there will be more plan diffs, so I think now we can turn if off in `saffron.properties` in this PR.
When top-down rule apply is committed into master, we need to come up with a solution to test the whole test suites with top-down on by default, but allow different plans.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429583933
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -969,7 +969,7 @@ private void checkJoinNWay(int n) throws Exception {
RelNode transform = planner.transform(0, traitSet, convert);
assertThat(toString(transform),
containsString(
- "EnumerableMergeJoin(condition=[=($0, $5)], joinType=[inner])"));
+ "EnumerableHashJoin(condition=[=($0, $5)], joinType=[inner])"));
Review comment:
TpcdsTest.testQuery17Plan also failed, which is expected. You can checkout TpcdsTest.java from any commit before I introduced top-down.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428877831
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
##########
@@ -159,7 +159,10 @@
/** Whether to make create implicit functions if functions do not exist
* in the operator table, default false. */
- LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false);
+ LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false),
+
+ /** Whether to enable top-down optimization in Volcano planner. */
+ VOLCANO_TOPDOWN_OPT("enableVolcanoTopDownOptimization", Type.BOOLEAN, false, false);
Review comment:
Removed `VOLCANO_`
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429569892
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
##########
@@ -199,4 +199,8 @@ public boolean lenientOperatorLookup() {
return CalciteConnectionProperty.LENIENT_OPERATOR_LOOKUP.wrap(properties)
.getBoolean();
}
+
+ public boolean topDownOpt() {
Review comment:
This parameter actually does not overwrite `saffron.properties`. The comment below.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431602181
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
Let's not consider `cast` in this PR:
```java
int maxField = Math.max(inputFieldCount, projects.size());
Mappings.TargetMapping mapping = Mappings
.create(MappingType.FUNCTION, maxField, maxField);
for (int i = 0; i < projects.size(); i++) {
RexNode exp = projects.get(i);
if (exp instanceof RexInputRef) {
mapping.set(((RexInputRef) exp).getIndex(), i);
}
}
```
If you check the body of `permutationIgnoreCast`, they are just reversed mapping.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431604532
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
I see. So basically the correctness of ordering is just partial sequence ordering that counts from beginning.
Say sort on [a, b, c, d], then it only guarantees [a], [a, b], [a, b, c] and [a, b, c, d].
And the rational is any non-trivial expr will break the ordering guarantees, thus produce a wrong 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580374
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
##########
@@ -159,7 +159,10 @@
/** Whether to make create implicit functions if functions do not exist
* in the operator table, default false. */
- LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false);
+ LENIENT_OPERATOR_LOOKUP("lenientOperatorLookup", Type.BOOLEAN, false, false),
+
+ /** Whether to enable top-down optimization in Volcano planner. */
+ TOPDOWN_OPT("topDownOpt", Type.BOOLEAN, false, false);
Review comment:
This is a good idea. I made this change. Also see comments above about further discussions.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580332
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,38 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
Review comment:
Done
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,38 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
Review comment:
Done. Added one test 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:
users@infra.apache.org
[GitHub] [calcite] hsyuan closed pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #1985:
URL: https://github.com/apache/calcite/pull/1985
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428387288
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
That makes sense, because it is cheaper.
Let's think it another way.
```
select a, b, c
from (select a, b, max(c) c from foo group by a, b) as t
where a+b+c>1
order by b desc, a desc;
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635792051
Thanks!
Commented `OptimizeTask.java:231` and logged it into CALCITE-4030.
Also squash all commits and add a detailed commit message on 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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-633143789
Made some changes into two commits to better illustrate changes.
Commit one includes
1. Have more rules removed in `Query()`. E.g. always use sorted aggregation, etc.
2. some style changes. E.g. use `required.getCollation()`.
3. For `testSortFilter` test case, used suggested SQL query (the one that includes an aggregation).
4. Added a new test case for sort-projection, which demonstrates in which case traits won't be propagated (e.g. order by CAST() where this cast does not preserve monotonicity, which is defined in https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java#L65)
Commit two is the change that set top-down opt globally false by default, include:
1. `calcite.planner.topdown.opt=false` in saffron.properties.
2. Some planner tests rolled back (from the change introduced by https://issues.apache.org/jira/browse/CALCITE-3896)
3. `CalciteConnectionProperty#TOPDOWN_OPT`'s default value is set by `CalciteSystemProperty.TOPDOWN_OPT.value()`.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635132405
Can you rebase on latest 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429584360
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -969,7 +969,7 @@ private void checkJoinNWay(int n) throws Exception {
RelNode transform = planner.transform(0, traitSet, convert);
assertThat(toString(transform),
containsString(
- "EnumerableMergeJoin(condition=[=($0, $5)], joinType=[inner])"));
+ "EnumerableHashJoin(condition=[=($0, $5)], joinType=[inner])"));
Review comment:
Fixed.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431469982
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
Now I change the implementation to:
1. traits pass through: return null when at least one collation is defined on non-trivial expr.
2. traits derivation: only return null when all collations are defined on non-trivial exprs. Otherwise derive those collations that are defined on trivial expr.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428389453
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Trait enforcement doesn't mean the relnode generated by passing through required traits is definitely best, it just adds an alternative.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429581868
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
Because some query or tests didn't add any trait defs. It is so bad decision to make Collation trait optional ❌. `ORDER BY` is sql standard, without supporting it, how can we call this a query optimizer? More freedom doesn't always make things better.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-636138841
@amaliujia Thank you for your hard work. I have committed with some modifications since some tests still throw exception like `RelBuilderTest.testRun()`.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429570185
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
Sorry, I mean here will overwrite. :)
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r432203107
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +73,23 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ if (required.getCollation() == null) {
Review comment:
Or you can simply try this
```
if (required.isDefaultSansConvention()) {
return null;
}
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-631866948
Comments addressed!
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635597802
@hsyuan really thanks for your patient explanations in this PR. This is the first time that I work on optimizer related code in Calcite and there are lots of details that I wasn't familiar with. Without your help, I couldn't go this 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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635761905
After turning on globally top-down opt, except for those tests that fail due to plan changes, there are some more test failed due to assert error.
Reason: `java.lang.AssertionError: cannot merge join: left input is not sorted on left keys`
Tests:
```
JdbcTest. testJoinInCorrelatedSubQuery()
JdbcTest. testLateral()
JdbcTest. testMergeJoin()
```
Reason: `java.lang.AssertionError
at org.apache.calcite.plan.volcano.OptimizeTask$RelNodeOptTask.execute(OptimizeTask.java:231)`
Tests:
```
SortRemoveRuleTest. removeSortOverEnumerableHashJoin()
SortRemoveRuleTest. removeSortOverEnumerableNestedLoopJoin()
SortRemoveRuleTest. removeSortOverEnumerableSemiJoin()
MaterializedViewRelOptRulesTest. testJoinAggregateMaterializationAggregateFuncs11()
MaterializedViewRelOptRulesTest. testJoinAggregateMaterializationNoAggregateFuncs7()
MaterializedViewRelOptRulesTest. testJoinAggregateMaterializationNoAggregateFuncs9()
MaterializedViewRelOptRulesTest. testJoinMaterialization10()
MaterializedViewRelOptRulesTest. testJoinMaterialization12()
MaterializedViewRelOptRulesTest. testJoinMaterializationUKFK5()
MaterializedViewRelOptRulesTest. testJoinMaterializationUKFK6()
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431442043
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
lot of thanks for your explanation. Now I understand it more:
Trait derivation can pass collations up when such collations are defined on trivial exprs.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431609986
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
Thanks. Updated.
Later we could add util functions for such reverse operations.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-631788216
R: @hsyuan PR updated. Thanks for your help.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428878347
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -65,92 +70,184 @@
* </ol>
*/
class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
+ // Remove to Keep deterministic join order
+ private static final List<RelOptRule> NON_DETERMINISTIC_JOIN_ORDER_RULES =
+ ImmutableList.of(
+ JoinCommuteRule.INSTANCE,
+ JoinPushThroughJoinRule.LEFT,
+ JoinPushThroughJoinRule.RIGHT
+ );
+
+ // Ordering enforcement should not be done by rules
+ private static final List<RelOptRule> SORT_PROJECT_TRANSPOSE_RULES =
+ ImmutableList.of(
+ SortProjectTransposeRule.INSTANCE,
+ ProjectSortTransposeRule.INSTANCE
+ );
+
+ // Remove to always use merge join
+ private static final List<RelOptRule> NON_MERGE_JOIN_RULES =
+ ImmutableList.of(
+ EnumerableRules.ENUMERABLE_JOIN_RULE,
+ EnumerableRules.ENUMERABLE_CALC_RULE
+ );
@Test void testSortAgg() {
final String sql = "select mgr, count(*) from sales.emp\n"
+ "group by mgr order by mgr desc nulls last limit 5";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE).check();
}
@Test void testSortAggPartialKey() {
final String sql = "select mgr,deptno,comm,count(*) from sales.emp\n"
+ "group by mgr,deptno,comm\n"
+ "order by comm desc nulls last, deptno nulls first";
- sql(sql).check();
+ // Always use sorted agg
+ Query.create(sql)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testSortMergeJoin() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by r.job desc nulls last, r.ename nulls first";
- sql(sql).check();
+ Query.create(sql).removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES).check();
}
@Test void testSortMergeJoinRight() {
final String sql = "select * from\n"
+ "sales.emp r join sales.bonus s on r.ename=s.ename and r.job=s.job\n"
+ "order by s.job desc nulls last, s.ename nulls first";
- sql(sql).check();
+ Query.create(sql).removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES).check();
}
@Test void testMergeJoinDeriveLeft1() {
final String sql = "select * from\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveLeft2() {
final String sql = "select * from\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "join sales.bonus s on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight1() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, max(sal) from sales.emp group by ename, job) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
+ .removeRules(NON_MERGE_JOIN_RULES)
+ .removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE)
+ .addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE)
+ .check();
}
@Test void testMergeJoinDeriveRight2() {
final String sql = "select * from sales.bonus s join\n"
+ "(select ename, job, mgr, max(sal) from sales.emp group by ename, job, mgr) r\n"
+ "on r.job=s.job and r.ename=s.ename";
- sql(sql).check();
+ Query.create(sql)
+ .removeRules(NON_DETERMINISTIC_JOIN_ORDER_RULES)
Review comment:
Agreed. Removed these rules for all tests now (as default).
If later any tests have a need to enable it, such tests can just add relavants rules.
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
##########
@@ -78,4 +78,6 @@
boolean typeCoercion();
/** @see CalciteConnectionProperty#LENIENT_OPERATOR_LOOKUP */
boolean lenientOperatorLookup();
+ /** @see CalciteConnectionProperty#VOLCANO_TOPDOWN_OPT */
+ boolean enableVolcanoTopDownOptimization();
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431867202
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
There is. You can use `Project.getPartialMapping()` instead.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430114503
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
I think the` size == 0` check is not necessary. Did you try remove it?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
Should it just childTraits?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
same here
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
+ }
+
+ @Override public DeriveMode getDeriveMode() {
+ return DeriveMode.BOTH;
Review comment:
why both? it has only 1 child, the default mode should be enough.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
Review comment:
Any positive case? I only see negative case.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
And it will be nice if we consider SqlMonotonicity here.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
+ }
+
+ @Override public DeriveMode getDeriveMode() {
+ return DeriveMode.BOTH;
Review comment:
For single child rel, LEFT_FIRST is enough. See DeriveMode's javadoc.
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,136 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive">
+ <Resource name="sql">
+ <![CDATA[
+select * from
+(select ename, job, max_sal from
+(select ename, job, max(sal) as max_sal from sales.emp group by ename, job) t) r
+join sales.bonus s on r.job=s.job and r.ename=s.ename
+]]>
Review comment:
Does this query want to test the trait derivation for Project?
If you remove the trait derivation code for Project, I guess the plan might be the same?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
This is not correct.
```
select distinct a, b*2, c from (select a, b, c from foo order by a, b, c limit 100) t;
```
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
The input is sorted by a, b, c.
The project is a, b*2, c
Based on this logic, the project is sorted by a, b*2, c. But in fact it is only sorted by a.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
The input is sorted by a, b, c.
The project is a, b\*2, c
Based on this logic, the project is sorted by a, b\*2, c. But in fact it is only sorted by a.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,48 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
Review comment:
Do we have a test case for this?
Say we have a project a, b+1 as b, c.
The parent request trait collation on a, b, c.
In this case, we can't pass it down.
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,136 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive">
+ <Resource name="sql">
+ <![CDATA[
+select * from
+(select ename, job, max_sal from
+(select ename, job, max(sal) as max_sal from sales.emp group by ename, job) t) r
+join sales.bonus s on r.job=s.job and r.ename=s.ename
+]]>
Review comment:
The project is removable. We can change
`select ename, job, max_sal from` to `select ename, job, max_sal+1 from`
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,136 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive">
+ <Resource name="sql">
+ <![CDATA[
+select * from
+(select ename, job, max_sal from
+(select ename, job, max(sal) as max_sal from sales.emp group by ename, job) t) r
+join sales.bonus s on r.job=s.job and r.ename=s.ename
+]]>
Review comment:
We are almost done. Project's trait propagation is tricky, unlike filter.
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -64,93 +69,196 @@
* <li>Run the test one last time; this time it should pass.
* </ol>
*/
-class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
-
+class TopDownOptTest {
@Test void testSortAgg() {
final String sql = "select mgr, count(*) from sales.emp\n"
+ "group by mgr order by mgr desc nulls last limit 5";
- sql(sql).check();
+ // Always use sorted agg
Review comment:
Remove this comment.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
In OptimizeTask, it will call `equalsSansConvention` before derive traits, so I guess it won't come here. Unless some other tests use Enumerable to test distribution trait, in which case, collation might be empty.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
No matter if has `order by a` or not, we need to see the plan:
```
SortedAgg
-> Sort
-> Project
-> Sort with limit
-> TableScan
```
In your current logic, the plan is:
```
SortedAgg
-> Project
-> Sort with limit
-> TableScan
```
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
And we'd better add another piece of logic to deal with SqlMonotonicity in traits derivation, similar with pass down.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430885788
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,136 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive">
+ <Resource name="sql">
+ <![CDATA[
+select * from
+(select ename, job, max_sal from
+(select ename, job, max(sal) as max_sal from sales.emp group by ename, job) t) r
+join sales.bonus s on r.job=s.job and r.ename=s.ename
+]]>
Review comment:
`max_sal+1` works like a charm.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429585612
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
We have to use a query that the default planner, even with all the rules are enabled, still can't generate as good plan as top-down optimizer.
I think the query in this test is not the one I showed you.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431072268
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,25 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
Review comment:
We can replace `collation` with `required.getCollation()`.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
Review comment:
Rename to `IsCollationOnTrivialExpr()`, and reverse the return value.
```
if (IsCollationOnTrivialExpr()) {
return Pair.of();
} else {
return null;
}
```
Not succinct, but clear.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
Here is something `derive` works differently with `passThrough`.
When the parent request a collation on [a,b,c], if b is a expr, say `a*k`, we will find that only `a` is mappable. But we can't pass down [a], because even the project can deliver collation on [a], it still can't satisfy [a,b,c], so Sort will be enforced. That will be a waste.
But if the child of Project delivers collation on [a,b,c], and the overall project expressions are (a, b, c*k, d, e), we can derive collation on [a,b] for the Project operator. This might be useful for parent operator. (Might still be wasteful, we'd better consider the parent request when derivation, but I guess that is something can be framework's duty, in the future).
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,25 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
Review comment:
same here
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(childTraits.replace(newCollation), ImmutableList.of(childTraits));
+ }
+
+ private boolean checkIfSortIsDefinedOnNonTrivalExpr(
+ Mappings.TargetMapping map, RelCollation collation) {
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
Review comment:
comment should be outside of 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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429582563
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
What does this query want to 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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428388218
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
In many cases, Sort should always be put on top of Filter. But in this case, we might get better plan by pushing down sort requirement past Filter.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431184692
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
For the last 2 negative test cases, I think you already have similar ones in your 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431469217
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -64,93 +69,243 @@
* <li>Run the test one last time; this time it should pass.
* </ol>
*/
-class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
-
+class TopDownOptTest {
Review comment:
Added `extends RelOptTestBase`
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430913096
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
Have added the logic to deal with SqlMonotonicity for traits derivation, and add one positive test case `testSortProjectDeriveWhenCastLeadingToMonotonic` and one negative test case `testSortProjectDeriveWhenCastLeadingToNonMonotonic`
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431314094
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
That's a great point. I also realized new tests's names cannot explain their purpose already. I was thinking a better way.
Will add more details comments.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #1985: [WIP][CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-631278902
@hsyuan
This PR is what my understanding about traits pass down:
EnumerableProject creates a new physical operator (EnumerableSort) based on requiredTrait, later it becomes a new Ruleset in planner, which is eligible for selection. By disabling several rules, this traits pass down code will generate the only usable plan (project at top, then a sort, and after that a scan)
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431465794
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -64,93 +69,243 @@
* <li>Run the test one last time; this time it should pass.
* </ol>
*/
-class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
-
+class TopDownOptTest {
Review comment:
Checkout this issue: CALCITE-4027
I think we still needs `extends RelOptTestBase` if we want to leverage it. Otherwise we have to manually copy from `TopDownOptTest_actual.xml`.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431465794
##########
File path: core/src/test/java/org/apache/calcite/test/TopDownOptTest.java
##########
@@ -64,93 +69,243 @@
* <li>Run the test one last time; this time it should pass.
* </ol>
*/
-class TopDownOptTest extends RelOptTestBase {
-
- protected DiffRepository getDiffRepos() {
- return DiffRepository.lookup(TopDownOptTest.class);
- }
-
- Sql sql(String sql) {
- VolcanoPlanner planner = new VolcanoPlanner();
- // Always use top-down optimization
- planner.setTopDownOpt(true);
- planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
- planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
-
- RelOptUtil.registerDefaultRules(planner, false, false);
-
- // Keep deterministic join order
- planner.removeRule(JoinCommuteRule.INSTANCE);
- planner.removeRule(JoinPushThroughJoinRule.LEFT);
- planner.removeRule(JoinPushThroughJoinRule.RIGHT);
-
- // Always use merge join
- planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE);
- planner.removeRule(EnumerableRules.ENUMERABLE_CALC_RULE);
-
- // Always use sorted agg
- planner.removeRule(EnumerableRules.ENUMERABLE_AGGREGATE_RULE);
- planner.addRule(EnumerableRules.ENUMERABLE_SORTED_AGGREGATE_RULE);
-
- Tester tester = createTester().withDecorrelation(true)
- .withClusterFactory(cluster -> RelOptCluster.create(planner, cluster.getRexBuilder()));
-
- return new Sql(tester, sql, null, planner,
- ImmutableMap.of(), ImmutableList.of());
- }
-
+class TopDownOptTest {
Review comment:
Checkout this issue: CALCITE-4027
I think we still need `extends RelOptTestBase` if we want to leverage it. Otherwise we have to manually copy from `TopDownOptTest_actual.xml`.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431120503
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
```sql
-- no sort for order by
select a, b*2 as d, c from (select distinct a, b, c from foo) order by a;
```
```sql
-- b is int, no sort for order by
select a, cast(b as bigint) as d, c from (select distinct a, b, c from foo) order by a, d;
```
```sql
-- b is int, need sort for order by
select a, cast(b as varchar) as d, c from (select distinct a, b, c from foo) order by a, d;
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431484608
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
I think a wrong plan is, even though inner sort provide the required ordering, the top projection still enforces a sort (so not 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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635762697
For the `OptimizeTask.java:231`, you can remove the assertion for now. I think there is set merge happened.
For the mergejoin ,there is an issue: CALCITE-4007
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431610131
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
Now it generates a correct plan (sort enforced for top projection).
Also changed this test case to `select a, b*-1, c` pattern for better testing.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431483152
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
The plan is wrong
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431120503
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
The test is difficult to come up with for derivation. :)
```sql
-- b is int, no sort for join's left input
select * from
(select a, cast(b as varchar) as d, c+1 as c from
(select * from foo order by a desc, b desc, c limit 10) t) foo
join bar
on foo.a = bar.a;
```
```sql
-- b is int, no sort for join's left input
select * from
(select a, cast(b as bigint) as d, c+1 as c from
(select * from foo order by a desc, b desc, c limit 10) t) foo
join bar
on foo.a = bar.a and foo.d = bar.d;
```
```sql
-- b is int, need sort for join's left input
select * from
(select a, cast(b as varchar) as d, c+1 as c from
(select * from foo order by a desc, b desc, c limit 10) t) foo
join bar
on foo.a = bar.a and foo.d = bar.d;
```
```sql
-- b is int, need sort for join's left input
select * from
(select a, b*-2 as d, c from
(select * from foo order by a desc, b desc, c limit 10) t) foo
join bar
on foo.a = bar.a and foo.d = bar.d;
```
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429056354
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,38 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
Review comment:
There is no test case covering 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429584061
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Test whether EnumerableFilter can push down collation.
To refresh memory a bit, this test used to generated something like the following by a single select query
```
EnemrableProject
EnumerableSort
EnumerableFilter
...
```
Though the plan above makes sense, but I think we want to see a generated plan that indeed have Sort enforced. Thus this test is updated.
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431559976
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
For 2, say the input is sorted by a,b,c.
There is a project on top of that, select a, b*-1 as d, c.
With your approach, it will derive collation on [a, c], which is wrong. Its derived collation should be just [a]. Any non-trivial expr will break the 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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635658810
@amaliujia You did a great job, far better than what I did before.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431610131
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,65 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+ if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
+ return null;
Review comment:
Now it generates a correct plan (sort enforced for top projection).
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635766640
For the `OptimizeTask.java:231`, let's comment it out instead of delete it. As a todo item, I need to investigate more.
Other than that, I am good with current change. +1 👍
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429584061
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Test whether EnumerableFilter can push down collation.
To refresh memory a bit, this test used to generated something like the following by a single select query
```
EnemrableProject
EnumerableSort
EnumerableFilter
...
```
Though the plan above makes sense, but I think we want to see a generated plan that indeed have Sort enforced. Thus this tes is updated.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429570352
##########
File path: core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java
##########
@@ -431,6 +431,7 @@ protected RelOptPlanner createPlanner(
if (CalciteSystemProperty.ENABLE_COLLATION_TRAIT.value()) {
planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
}
+ planner.setTopDownOpt(prepareContext.config().topDownOpt());
Review comment:
My current understanding is, we want to make sure Top-down optimization does not affect existing implementation.
Then, not sure if you are aware of that, the value in `saffron.properties` always overwrite `CalciteSystemProperty.TOPDOWN_OPT`, thus `CalciteSystemProperty.TOPDOWN_OPT` is always true (at least in JDBC code path when I tried to debug JDDC test failures).
Thus, top-down optimizations are enabled by default in many tests. And the only way to update it is calling `planner.setTopDownOpt`.
This line is trying to disable top-down opt by default for JDBC code path (e.g. JDBC tests). Without this linke, top-down opt is enabled and ~40 tests are failing due to trait propagation implementation in this PR.
So do I understand correctly that top-down opt should be implemented right now in a relative isolated environment? Can we set the value in `saffron.properties` as false to turn off top-down opt by default? Is there better suggestions?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580983
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Now traits propagation in EnumerableFilter generates a plan that looks reasonable.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431484135
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Why?
The top project does not need to enforce a sort because inner sort has provide the needed ordering.
Or I am still misunderstanding the purpose of deriving partial working traits for projection?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430092458
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Updated this `testSortFilter`. Without top-down apt, the query generates
```
EnumerableSort(sort0=[$2], sort1=[$0], dir0=[DESC], dir1=[ASC])
EnumerableFilter(condition=[>($3, 1000)])
EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```,
and with top-down opt,
it generates
```
EnumerableFilter(condition=[>($3, 1000)])
EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
EnumerableSort(sort0=[$3], sort1=[$1], sort2=[$2], dir0=[DESC], dir1=[ASC], dir2=[ASC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Updated this `testSortFilter`. Without top-down apt, the query generates
```
EnumerableSort(sort0=[$2], sort1=[$0], dir0=[DESC], dir1=[ASC])
EnumerableFilter(condition=[>($3, 1000)])
EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
and with top-down opt,
it generates
```
EnumerableFilter(condition=[>($3, 1000)])
EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
EnumerableSort(sort0=[$3], sort1=[$1], sort2=[$2], dir0=[DESC], dir1=[ASC], dir2=[ASC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
There were empty collation cases (surprisingly but there were some).
That's why I have to check this(otherwise later it will fail)
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
+ }
+
+ @Override public DeriveMode getDeriveMode() {
+ return DeriveMode.BOTH;
Review comment:
Ah misunderstood what this mode controls. Agreed the default mode is enough.
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
See comment above.
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,136 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive">
+ <Resource name="sql">
+ <![CDATA[
+select * from
+(select ename, job, max_sal from
+(select ename, job, max(sal) as max_sal from sales.emp group by ename, job) t) r
+join sales.bonus s on r.job=s.job and r.ename=s.ename
+]]>
Review comment:
Ah the plan is the same even if remove trait derivation implementation. Honestly I don't have an explanation (still learning top-down optimization).
The intention here is I explicitly add another project between the left sorted aggregation and join, to test if the sort collation is derived by Join thus Join can enforce right input has the same collation. (e.g. I add an extra `select ename, job, max_sal from` on top of the left aggregation).
Any comment or suggestion on how should a project trait derivation should be properly tested?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
Could you explain a bit on why this is not correct based on your query?
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +75,29 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ // Filter, not like Project, does not change on which column that collation defines.
+ return Pair.of(required, ImmutableList.of(required));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
Ok I tried to remove all `size()==0` and it works.
So i will remove such checks. (It's still wired to me but will let it go).
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
Review comment:
Added one positive case (e.g. order by cast() is involved but cast() is monotonic).
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
Gotcha. I think your example query misses `order by a` at the end.
Full query
```
select distinct a, b*2, c
from (select a, b, c from foo order by a, b, c limit 100) t
order by a;
```
Let me think about 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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428361297
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
I setup debugger and conformed that there was a `EnumerableSort` enforced for `EnumerableFilter`, however, I wasn't sure why planner always think that this plan is cheapest. In fact, I was hoping to see the following plan:
```
EnumerableProject(MGR=[$3])
EnumerableFilter(condition=[>($7, 10)])
EnumerableSort(sort0=[$3], dir0=[DESC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429585112
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,73 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectWhenCastLeadingToNoTMonotonic">
+ <Resource name="sql">
+ <![CDATA[select deptno from sales.emp order by cast(deptno as varchar) desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], dir0=[DESC])
+ LogicalProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$1], dir0=[DESC])
+ EnumerableProject(DEPTNO=[$7], EXPR$1=[CAST($7):VARCHAR NOT NULL])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[
+select ename, job, mgr, max_sal from
+(select ename, job, mgr, max(sal) as max_sal from sales.emp group by ename, job, mgr) as t
+where max_sal > 1000
+order by mgr desc, max_sal
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ LogicalProject(ENAME=[$0], JOB=[$1], MGR=[$2], MAX_SAL=[$3])
+ LogicalFilter(condition=[>($3, 1000)])
+ LogicalAggregate(group=[{0, 1, 2}], MAX_SAL=[MAX($3)])
+ LogicalProject(ENAME=[$1], JOB=[$2], MGR=[$3], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableSort(sort0=[$2], sort1=[$3], dir0=[DESC], dir1=[ASC])
+ EnumerableFilter(condition=[>($3, 1000)])
+ EnumerableSortedAggregate(group=[{1, 2, 3}], MAX_SAL=[MAX($5)])
+ EnumerableSort(sort0=[$1], sort1=[$2], sort2=[$3], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
If you remove
```
.removeRule(EnumerableRules.ENUMERABLE_SORT_RULE)
```
and run with top-down disabled,
it can generate the same plan.
The Sort it not pushed down below Filter.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580988
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580895
##########
File path: core/src/test/java/org/apache/calcite/tools/PlannerTest.java
##########
@@ -969,7 +969,7 @@ private void checkJoinNWay(int n) throws Exception {
RelNode transform = planner.transform(0, traitSet, convert);
assertThat(toString(transform),
containsString(
- "EnumerableMergeJoin(condition=[=($0, $5)], joinType=[inner])"));
+ "EnumerableHashJoin(condition=[=($0, $5)], joinType=[inner])"));
Review comment:
Changes in this class rolls back some changes in https://github.com/apache/calcite/commit/bb484859e115596061c572b689a3b04349299458, because now we turns off top-down opt globally.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431482634
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -128,36 +132,42 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
final Mappings.TargetMapping map =
RelOptUtil.permutationIgnoreCast(
getProjects(), getInput().getRowType());
- if (checkIfSortIsDefinedOnNonTrivalExpr(map, collation)) {
- return null;
+
+ List<RelFieldCollation> collationFieldsToDerive = new ArrayList<>();
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (isCollationOnTrivialExpr(map, rc)) {
+ collationFieldsToDerive.add(rc);
+ }
Review comment:
This is a good test case. Added (see `testSortProjectDerive5`)
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635702425
You may also need this. I remember you already did this before, and changed it per my request.
```diff
diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
index a30328f42..9eb4c486a 100644
--- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
+++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
@@ -18,6 +18,7 @@
import org.apache.calcite.plan.RelOptCluster;
import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelCollation;
import org.apache.calcite.rel.RelCollationTraitDef;
import org.apache.calcite.rel.RelDistributionTraitDef;
import org.apache.calcite.rel.RelNode;
@@ -76,20 +77,22 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
@Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
RelTraitSet required) {
- if (required.getCollation() == null) {
+ if (required.isDefaultSansConvention()) {
return null;
}
// Filter, not like Project, does not change on which column that collation defines.
- return Pair.of(required, ImmutableList.of(required));
+ RelCollation collation = required.getCollation();
+ RelTraitSet traits = traitSet.replace(collation);
+ return Pair.of(traits, ImmutableList.of(traits));
}
@Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
final RelTraitSet childTraits, final int childId) {
- if (childTraits.getCollation() == null) {
+ if (childTraits.isDefaultSansConvention()) {
return null;
}
-
- return Pair.of(childTraits, ImmutableList.of(childTraits));
+ RelCollation collation = childTraits.getCollation();
+ return Pair.of(traitSet.replace(collation), ImmutableList.of(childTraits));
}
}
diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
index 153e0d6ae..2d8cf7cf5 100644
--- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
+++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
@@ -107,7 +107,7 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
@Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
RelTraitSet required) {
RelCollation collation = required.getCollation();
- if (collation == null) {
+ if (required.isDefaultSansConvention()) {
return null;
}
@@ -122,13 +122,14 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
}
final RelCollation newCollation = collation.apply(map);
- return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ return Pair.of(traitSet.replace(collation),
+ ImmutableList.of(traitSet.replace(newCollation)));
}
@Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
final RelTraitSet childTraits, final int childId) {
RelCollation collation = childTraits.getCollation();
- if (collation == null) {
+ if (childTraits.isDefaultSansConvention()) {
return null;
}
@@ -157,7 +158,8 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
if (collationFieldsToDerive.size() > 0) {
final RelCollation newCollation = RelCollations.of(collationFieldsToDerive).apply(mapping);
- return Pair.of(childTraits.replace(newCollation), ImmutableList.of(childTraits));
+ return Pair.of(traitSet.replace(newCollation),
+ ImmutableList.of(childTraits));
} else {
return null;
}
```
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430922984
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,52 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
+ }
+ final RexNode node = getProjects().get(fc.getFieldIndex());
+ if (node.isA(SqlKind.CAST)) {
+ // Check whether it is a monotonic preserving cast, otherwise we cannot push
+ final RexCall cast = (RexCall) node;
+ RelFieldCollation newFc = Objects.requireNonNull(RexUtil.apply(map, fc));
+ final RexCallBinding binding =
+ RexCallBinding.create(getCluster().getTypeFactory(), cast,
+ ImmutableList.of(RelCollations.of(newFc)));
+ if (cast.getOperator().getMonotonicity(binding) == SqlMonotonicity.NOT_MONOTONIC) {
+ return null;
+ }
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null || collation.getFieldCollations().size() == 0) {
+ return null;
+ }
+
+ return Pair.of(getTraitSet().replace(collation), ImmutableList.of(childTraits));
Review comment:
And regarding the distinct query, I added it as the query in test case `testSortProjectDerive2`, where it generates a plan:
```
EnumerableSortedAggregate(group=[{0, 1, 2}])
EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
EnumerableProject(ENAME=[$0], EXPR$1=[*($2, -2)], MGR=[$1])
EnumerableLimit(fetch=[100])
EnumerableProject(ENAME=[$1], MGR=[$3], SAL=[$5])
EnumerableSort(sort0=[$1], sort1=[$3], sort2=[$5], dir0=[ASC], dir1=[ASC], dir2=[ASC])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
```
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431486913
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
Bad plan is still correct plan. But this is incorrect plan.
The input is sorted by empno, ename, job .
We want the output sorted by ename, job.
empno | ename | job
------------ | ------------- | -------------
1 | 1 | 1
1 | 2 | 1
2 | 1 | 1
2 | 2 | 1
----------------------------------------------------------------
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
[GitHub] [calcite] hsyuan commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431561103
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +100,74 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ for (RelFieldCollation rc : collation.getFieldCollations()) {
+ if (!isCollationOnTrivialExpr(map, rc)) {
+ return null;
+ }
+ }
+
+ final RelCollation newCollation = collation.apply(map);
+ return Pair.of(required, ImmutableList.of(required.replace(newCollation)));
+ }
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> deriveTraits(
+ final RelTraitSet childTraits, final int childId) {
+ RelCollation collation = childTraits.getCollation();
+ if (collation == null) {
+ return null;
+ }
+
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
Review comment:
You can't use the same mapping. This is used to map parent to child. What you want is map child position to parent position.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r430908505
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableProject.java
##########
@@ -86,4 +99,48 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalcRel is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getCollation();
+ if (collation == null) {
+ return null;
+ }
+ final Mappings.TargetMapping map =
+ RelOptUtil.permutationIgnoreCast(
+ getProjects(), getInput().getRowType());
+
+ // Determine mapping between project input and output fields. If sort
+ // relies on non-trivial expressions, we can't push.
+ for (RelFieldCollation fc : collation.getFieldCollations()) {
+ if (map.getTargetOpt(fc.getFieldIndex()) < 0) {
+ return null;
Review comment:
Good point. Add one test case for trait push down `testSortProjectOnRexCall` and one for trait derivation `testSortProjectDeriveOnRexCall`
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r428430702
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -35,6 +35,46 @@ EnumerableLimit(fetch=[5])
EnumerableSortedAggregate(group=[{3}], EXPR$1=[COUNT()])
EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProject">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp order by mgr desc nulls last]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC-nulls-last])
+ LogicalProject(MGR=[$3])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC-nulls-last])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortFilter">
+ <Resource name="sql">
+ <![CDATA[select mgr from sales.emp where deptno > 10 order by mgr desc]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$0], dir0=[DESC])
+ LogicalProject(MGR=[$3])
+ LogicalFilter(condition=[>($7, 10)])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(MGR=[$3])
+ EnumerableSort(sort0=[$3], dir0=[DESC])
+ EnumerableFilter(condition=[>($7, 10)])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
That's a good point that filter first then sort after, since filter will reduce volume of data thus reduce cost of sort. (not absolute but make senses).
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia edited a comment on pull request #1985: [WIP][CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-631278902
@hsyuan
This PR is what my understanding about traits pass down:
EnumerableProject creates a new physical operator (EnumerableSort) based on requiredTrait, later it becomes a new Ruleset in planner, which is eligible for selection. By disabling several rules, this traits pass down code will generate the only usable plan (project at top, then a sort, and after that a scan).
Any comment on if I am understanding it correctly?
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r431484608
##########
File path: core/src/test/resources/org/apache/calcite/test/TopDownOptTest.xml
##########
@@ -318,6 +318,31 @@ EnumerableProject(ENAME=[$0], JOB=[$1], EXPR$2=[$2], ENAME0=[$3], JOB0=[$4], SAL
EnumerableSort(sort0=[$4], sort1=[$0], dir0=[ASC], dir1=[ASC])
EnumerableProject(ENAME=[$0], JOB=[$1], SAL=[$2], COMM=[$3], JOB0=[CAST($1):BIGINT NOT NULL])
EnumerableTableScan(table=[[CATALOG, SALES, BONUS]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testSortProjectDerive5">
+ <Resource name="sql">
+ <![CDATA[
+select empno*-1, ename, job from
+(select * from sales.emp order by empno, ename, job limit 10) order by ename, job
+]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalSort(sort0=[$1], sort1=[$2], dir0=[ASC], dir1=[ASC])
+ LogicalProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ LogicalSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC], fetch=[10])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+EnumerableProject(EXPR$0=[*($0, -1)], ENAME=[$1], JOB=[$2])
+ EnumerableLimit(fetch=[10])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC], dir2=[ASC])
+ EnumerableTableScan(table=[[CATALOG, SALES, EMP]])
Review comment:
I think a wrong plan is even though inner sort provide the required ordering, the top projection still enforces a sort (so not 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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-635136029
Rebased on 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
[GitHub] [calcite] amaliujia commented on pull request #1985: [CALCITE-4011] Support trait propagation for EnumerableProject and EnumerableFilter
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#issuecomment-636140486
Thank you!
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #1985: [CALCITE-4011] Implement trait propagation for EnumerableProject…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1985:
URL: https://github.com/apache/calcite/pull/1985#discussion_r429580627
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableFilter.java
##########
@@ -68,4 +74,15 @@ public Result implement(EnumerableRelImplementor implementor, Prefer pref) {
// EnumerableCalc is always better
throw new UnsupportedOperationException();
}
+
+ @Override public Pair<RelTraitSet, List<RelTraitSet>> passThroughTraits(
+ RelTraitSet required) {
+ RelCollation collation = required.getTrait(RelCollationTraitDef.INSTANCE);
+ if (collation == null || collation.getFieldCollations().size() == 0) {
Review comment:
Yes. I found similar code in `EnumerableConvention.enforce()`.
But, trait propagation in operators happens before `EnumerableConvention.enforce()`, thus EnumerableFilter and EnumerableProject will face first how to deal with this case.
If
1) assert. It means crash in operator, which is not ok.
2) if don't handle it, it leads NPE (when collation is null)
so seems best to me is return `null` when in such cases, which means cannot propagate traits (which also makes sense for null or empty collation).
And, I think to have a really good solution/reasoning, it's better to first understand where does null or empty collation generated, and why. (which I don't have an answer 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