terrymanu commented on issue #23014:
URL:
https://github.com/apache/shardingsphere/issues/23014#issuecomment-3636959686
## Issue Understanding
- In 5.1.2, a query like GROUP BY col1,col2 + ORDER BY col1 DESC,col2 DESC
gets rewritten to LIMIT 0,2147483647, causing full fetch, because
SelectStatementContext#isSameGroupByAndOrderByItems returns false.
- By SQL semantics, GROUP BY has no direction; if ORDER BY uses the same
column list in the same order, regardless of ASC/DESC, streaming merge should
still be valid and the original LIMIT can be preserved.
## Root Cause
- The current check includes sort direction: groupByContext treats
direction as default ASC, so a user-specified DESC is deemed “not consistent,”
triggering LIMIT expansion.
- This is overly conservative and makes “same columns, different
directions” fall back to full fetch.
## Analysis
- Per the official doc “Merge Engine”
(docs/document/content/reference/sharding/merge.en.md), streaming group merge
relies on grouping keys being the sorting keys so rows of the same group are
contiguous; direction differences do not break contiguity and should only be
honored in the comparator.
- Therefore, the consistency check should focus on column/expression
sequences, not on sort direction; direction should be applied during merge
comparison.
## Conclusion & Fix Proposal
- Conclusion: It’s a logic defect—sorting direction is used in the
consistency check, causing false negatives and LIMIT expansion.
- Fix idea: In isSameGroupByAndOrderByItems, compare only the
column/expression sequences (including order of columns), ignoring sort
direction; keep direction for the merge comparator. Add tests for GROUP BY with
matching column lists but DESC/mixed order to ensure
pagination is no longer expanded. Example pseudo-code:
```java
// Compare columns/expressions only, ignore direction
private boolean isSameGroupByAndOrderByItems() {
return groupByContext.getItems().size() ==
orderByContext.getItems().size()
&& IntStream.range(0, groupByContext.getItems().size())
.allMatch(i ->
isSameExpression(groupByContext.getItems().get(i),
orderByContext.getItems().get(i)));
}
```
- We warmly invite community contributors to submit a PR implementing this
change and adding the relevant tests; we’ll gladly assist with review.
--
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]
