xiedeyantu commented on code in PR #4344:
URL: https://github.com/apache/calcite/pull/4344#discussion_r2067598830
##########
core/src/main/java/org/apache/calcite/rel/rules/SortJoinTransposeRule.java:
##########
@@ -79,13 +80,18 @@ public SortJoinTransposeRule(Class<? extends Sort>
sortClass,
final JoinInfo joinInfo =
JoinInfo.createWithStrictEquality(join.getLeft(), join.getRight(),
join.getCondition());
- // 1) If join is not a left or right outer, we bail out
- // 2) If sort is not a trivial order-by, and if there is
+ // 1) If the fetch of sort is dynamic parameter, we bail out
+ // 2) If join is not a left or right outer, we bail out
+ // 3) If sort is not a trivial order-by, and if there is
// any sort column that is not part of the input where the
// sort is pushed, we bail out
- // 3) If sort has an offset, and if the non-preserved side
+ // 4) If sort has an offset, and if the non-preserved side
// of the join is not count-preserving against the join
// condition, we bail out
+ if (sort.fetch instanceof RexDynamicParam) {
Review Comment:
> Hi @xiedeyantu Currently we only handle left and right join, if only have
limit, sort push through preserved side didn't change sql result. If with
offset, it will more complicated, sort only push through to preserved side, and
the other side need count-preserving, this will not change the sql result.
>
> "pushdown" is incorrect, community use "push through" before, it means
that the original sort still there, and the sort will be add to the
corresponding place. Thus, multiple dynamic params may diff from the semantic
expression required by the original SQL when in Preparestatement as @NobiGo
mentioned. This is my understanding. Welcome any corrections。
Thanks for the comprehensive explanation. I also think this PR fix is
correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
