libenchao commented on code in PR #2848:
URL: https://github.com/apache/calcite/pull/2848#discussion_r915375337
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableLimit.java:
##########
@@ -96,11 +96,7 @@ public static EnumerableLimit create(final RelNode input,
@Nullable RexNode offs
final BlockBuilder builder = new BlockBuilder();
final EnumerableRel child = (EnumerableRel) getInput();
final Result result = implementor.visitChild(this, 0, child, pref);
- final PhysType physType =
- PhysTypeImpl.of(
- implementor.getTypeFactory(),
- getRowType(),
- result.format);
+ final PhysType physType = result.physType;
Review Comment:
The plan before this PR:
```xml
EnumerableLimit(offset=[?1], fetch=[?0])
EnumerableSort(sort0=[$3], dir0=[ASC])
EnumerableCalc(expr#0..6=[{inputs}], empid=[$t2], deptno=[$t3],
name=[$t4], salary=[$t5], commission=[$t6])
EnumerableHashJoin(condition=[AND(=($0, $2), =($1, $3))],
joinType=[inner])
EnumerableAggregate(group=[{0}], EXPR$0=[MAX($1)])
EnumerableTableScan(table=[[hr, emps]])
EnumerableTableScan(table=[[hr, emps]])
```
after this PR:
```xml
EnumerableSort(offset=[?1], fetch=[?0])
EnumerableSort(sort0=[$3], dir0=[ASC])
EnumerableHashJoin(condition=[AND(=($0, $5), =($1, $6))],
joinType=[semi])
EnumerableTableScan(table=[[hr, emps]])
EnumerableAggregate(group=[{0}], EXPR$0=[MAX($1)])
EnumerableTableScan(table=[[hr, emps]])
```
`EnumerableHashJoin(joinType=[semi])`'s physical type is `Employee`.
However, `EnumerableSort` and `EnumerableSort`'s physical type is `Record5_0`.
That's why after this PR, the test will fail.
> This change seems a bit ad-hoc. Why is it applied only on
{{EnumerableLimit}} and {{EnumerableSort}}, but we keep the "old code" in other
operators like {{EnumerableLimitSort}} or {{EnumerableAggregate}}?
You are right, we should check all places, or figure out another way to fix
this.
This is indeed orthogonal with current PR, maybe we can leave it as a
follow-up issue. I've considered this way before, there may be two ways to to
this:
* Remove `JoinOnUniqueToSemiJoinRule` from
`RelOptRules#ABSTRACT_RELATIONAL_RULES` and `Prorams#RULE_SET`
* Disable `JdbcTest#testDynamicParameterInLimitOffset` and link the
follow-up issue id to it
What do you think about these two ways?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
