Skip to content

Commit 9f19c7b

Browse files
terrymanuterrymanu
committed
Fix isSameGroupByAndOrderByItems to ignore order direction for group by and order by items
Judge group by and order by item sameness by select item identity, which is the resolved projection index or the segment text or column index, instead of the direction-sensitive OrderByItem equality. This keeps pagination rewrite using the original row count and group by stream merge eligible for queries like GROUP BY a, b ORDER BY a DESC, b DESC, and closes the latent wrong-result path where unresolved rewrite-time indexes made GROUP BY a ORDER BY SUM(c) ASC keep the per-shard limit. Fixes #23014
1 parent d85071e commit 9f19c7b

3 files changed

Lines changed: 136 additions & 16 deletions

File tree

‎infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/SelectStatementBaseContext.java‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.Projection;
3131
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.ProjectionsContext;
3232
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.engine.ProjectionsContextEngine;
33+
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.extractor.ProjectionIdentifierExtractEngine;
3334
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.impl.AggregationDistinctProjection;
3435
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.impl.AggregationProjection;
3536
import org.apache.shardingsphere.infra.binder.context.segment.select.projection.impl.ColumnProjection;
@@ -77,6 +78,7 @@
7778
import java.util.LinkedList;
7879
import java.util.List;
7980
import java.util.Map;
81+
import java.util.Objects;
8082
import java.util.Optional;
8183

8284
/**
@@ -357,7 +359,7 @@ private String getOrderItemText(final TextOrderByItemSegment orderByItemSegment)
357359
}
358360

359361
/**
360-
* Judge whether group by items and order by items refer to the same select items in the same sequence, ignoring order direction.
362+
* Judge whether group by items and order by items can be proven to refer to the same select items in the same sequence, ignoring order direction.
361363
*
362364
* <p>Either order direction keeps rows of the same group by keys adjacent in the merged result, so the direction-insensitive judgment
363365
* keeps pagination rewrite and group by stream merge available when the order by keys are exactly the group by keys.</p>
@@ -368,6 +370,9 @@ public boolean isSameGroupByAndOrderByItems() {
368370
if (groupByContext.getItems().isEmpty() || groupByContext.getItems().size() != orderByContext.getItems().size()) {
369371
return false;
370372
}
373+
if (orderByContext.isGenerated()) {
374+
return true;
375+
}
371376
Iterator<OrderByItem> groupByItems = groupByContext.getItems().iterator();
372377
Iterator<OrderByItem> orderByItems = orderByContext.getItems().iterator();
373378
while (groupByItems.hasNext()) {
@@ -379,18 +384,61 @@ public boolean isSameGroupByAndOrderByItems() {
379384
}
380385

381386
private boolean isSameGroupByAndOrderByItem(final OrderByItem groupByItem, final OrderByItem orderByItem) {
382-
// Resolved indexes are 1-based and point to select items; index 0 means the index has not been resolved yet.
383-
if (0 != groupByItem.getIndex() && groupByItem.getIndex() == orderByItem.getIndex()) {
384-
return true;
387+
if (0 != groupByItem.getIndex() && 0 != orderByItem.getIndex() && groupByItem.getIndex() != orderByItem.getIndex()) {
388+
return false;
385389
}
386390
OrderByItemSegment groupBySegment = groupByItem.getSegment();
387391
OrderByItemSegment orderBySegment = orderByItem.getSegment();
388-
if (groupBySegment instanceof IndexOrderByItemSegment && orderBySegment instanceof IndexOrderByItemSegment) {
392+
if (groupBySegment.getClass() != orderBySegment.getClass()) {
393+
return false;
394+
}
395+
if (groupBySegment instanceof IndexOrderByItemSegment) {
389396
return ((IndexOrderByItemSegment) groupBySegment).getColumnIndex() == ((IndexOrderByItemSegment) orderBySegment).getColumnIndex();
390397
}
391-
return groupBySegment instanceof TextOrderByItemSegment && orderBySegment instanceof TextOrderByItemSegment
392-
&& SQLUtils.getExactlyValue(((TextOrderByItemSegment) groupBySegment).getText())
393-
.equalsIgnoreCase(SQLUtils.getExactlyValue(((TextOrderByItemSegment) orderBySegment).getText()));
398+
if (groupBySegment instanceof ColumnOrderByItemSegment) {
399+
ColumnSegment groupByColumn = ((ColumnOrderByItemSegment) groupBySegment).getColumn();
400+
ColumnSegment orderByColumn = ((ColumnOrderByItemSegment) orderBySegment).getColumn();
401+
return isSameColumn(groupByColumn, orderByColumn);
402+
}
403+
return false;
404+
}
405+
406+
private boolean isSameColumn(final ColumnSegment groupByColumn, final ColumnSegment orderByColumn) {
407+
ColumnSegmentBoundInfo groupByBoundInfo = groupByColumn.getColumnBoundInfo();
408+
ColumnSegmentBoundInfo orderByBoundInfo = orderByColumn.getColumnBoundInfo();
409+
ColumnSegmentBoundInfo groupByOtherUsingBoundInfo = groupByColumn.getOtherUsingColumnBoundInfo();
410+
ColumnSegmentBoundInfo orderByOtherUsingBoundInfo = orderByColumn.getOtherUsingColumnBoundInfo();
411+
return groupByColumn.getQualifiedName().equals(orderByColumn.getQualifiedName()) && null != groupByBoundInfo && null != orderByBoundInfo
412+
&& isSameColumnBoundInfo(groupByBoundInfo, orderByBoundInfo)
413+
&& (null == groupByOtherUsingBoundInfo ? null == orderByOtherUsingBoundInfo
414+
: null != orderByOtherUsingBoundInfo && isSameColumnBoundInfo(groupByOtherUsingBoundInfo, orderByOtherUsingBoundInfo))
415+
&& !isAmbiguousProjectionReference(groupByColumn);
416+
}
417+
418+
private boolean isAmbiguousProjectionReference(final ColumnSegment column) {
419+
if (null != column.getNestedObjectAttributes() && !column.getNestedObjectAttributes().isEmpty()) {
420+
return true;
421+
}
422+
if (column.getOwner().isPresent()) {
423+
return false;
424+
}
425+
String identifier = new ProjectionIdentifierExtractEngine(sqlStatement.getDatabaseType()).getIdentifierValue(column.getIdentifier());
426+
for (Projection each : projectionsContext.getExpandProjections()) {
427+
if (!identifier.equalsIgnoreCase(each.getColumnLabel())) {
428+
continue;
429+
}
430+
if (each.getAlias().isPresent() || !(each instanceof ColumnProjection) || null == ((ColumnProjection) each).getColumnBoundInfo()
431+
|| !isSameColumnBoundInfo(column.getColumnBoundInfo(), ((ColumnProjection) each).getColumnBoundInfo())) {
432+
return true;
433+
}
434+
}
435+
return false;
436+
}
437+
438+
private boolean isSameColumnBoundInfo(final ColumnSegmentBoundInfo left, final ColumnSegmentBoundInfo right) {
439+
return null != left && null != right && left.getTableSourceType() == right.getTableSourceType()
440+
&& Objects.equals(left.getOriginalDatabase(), right.getOriginalDatabase()) && Objects.equals(left.getOriginalSchema(), right.getOriginalSchema())
441+
&& Objects.equals(left.getOriginalTable(), right.getOriginalTable()) && Objects.equals(left.getOriginalColumn(), right.getOriginalColumn());
394442
}
395443

396444
/**

‎infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/SelectStatementContextTest.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.AliasSegment;
5757
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.OwnerSegment;
5858
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.PivotSegment;
59+
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.bound.ColumnSegmentBoundInfo;
5960
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.bound.TableSegmentBoundInfo;
6061
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.table.JoinTableSegment;
6162
import org.apache.shardingsphere.sql.parser.statement.core.segment.generic.table.SimpleTableSegment;
@@ -191,6 +192,77 @@ void assertIsSameGroupByAndOrderByItemsWithColumnOrderByAndDifferentOrderDirecti
191192
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
192193
assertTrue(selectStatementContext.isSameGroupByAndOrderByItems());
193194
}
195+
196+
@Test
197+
void assertIsSameGroupByAndOrderByItemsWithGeneratedOrderBy() {
198+
SelectStatement selectStatement = SelectStatement.builder().databaseType(databaseType).projections(new ProjectionsSegment(0, 0))
199+
.groupBy(new GroupBySegment(0, 0, Collections.singletonList(
200+
new ExpressionOrderByItemSegment(0, 0, "LOWER(id)", OrderDirection.ASC, NullsOrderType.LAST))))
201+
.build();
202+
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
203+
assertTrue(selectStatementContext.isSameGroupByAndOrderByItems());
204+
}
205+
206+
@Test
207+
void assertIsNotSameGroupByAndOrderByItemsWhenResolvedExpressionIndexesCollide() {
208+
SelectStatement selectStatement = SelectStatement.builder().databaseType(databaseType).projections(new ProjectionsSegment(0, 0))
209+
.groupBy(new GroupBySegment(0, 0, Collections.singletonList(
210+
new ExpressionOrderByItemSegment(0, 0, "CONCAT('a,b', name, 'c')", OrderDirection.ASC, NullsOrderType.LAST))))
211+
.orderBy(new OrderBySegment(0, 0, Collections.singletonList(
212+
new ExpressionOrderByItemSegment(0, 0, "CONCAT('a', 'b,name', 'c')", OrderDirection.DESC, NullsOrderType.LAST))))
213+
.build();
214+
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
215+
selectStatementContext.getGroupByContext().getItems().iterator().next().setIndex(1);
216+
selectStatementContext.getOrderByContext().getItems().iterator().next().setIndex(1);
217+
assertFalse(selectStatementContext.isSameGroupByAndOrderByItems());
218+
}
219+
220+
@Test
221+
void assertIsNotSameGroupByAndOrderByItemsWhenExpressionsContainDifferentParameterMarkers() {
222+
SelectStatement selectStatement = SelectStatement.builder().databaseType(databaseType).projections(new ProjectionsSegment(0, 0))
223+
.groupBy(new GroupBySegment(0, 0, Collections.singletonList(new ExpressionOrderByItemSegment(
224+
0, 0, "MOD(id, ?)", OrderDirection.ASC, NullsOrderType.LAST, new ParameterMarkerExpressionSegment(0, 0, 0)))))
225+
.orderBy(new OrderBySegment(0, 0, Collections.singletonList(new ExpressionOrderByItemSegment(
226+
0, 0, "MOD(id, ?)", OrderDirection.DESC, NullsOrderType.LAST, new ParameterMarkerExpressionSegment(0, 0, 1)))))
227+
.build();
228+
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
229+
assertFalse(selectStatementContext.isSameGroupByAndOrderByItems());
230+
}
231+
232+
@Test
233+
void assertIsNotSameGroupByAndOrderByItemsWhenQuotedColumnCaseDiffers() {
234+
SelectStatement selectStatement = SelectStatement.builder().databaseType(databaseType).projections(new ProjectionsSegment(0, 0))
235+
.groupBy(new GroupBySegment(0, 0, Collections.singletonList(
236+
new ColumnOrderByItemSegment(new ColumnSegment(0, 0, new IdentifierValue("\"Foo\"")), OrderDirection.ASC, NullsOrderType.LAST))))
237+
.orderBy(new OrderBySegment(0, 0, Collections.singletonList(
238+
new ColumnOrderByItemSegment(new ColumnSegment(0, 0, new IdentifierValue("\"foo\"")), OrderDirection.DESC, NullsOrderType.LAST))))
239+
.build();
240+
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
241+
assertFalse(selectStatementContext.isSameGroupByAndOrderByItems());
242+
}
243+
244+
@Test
245+
void assertIsNotSameGroupByAndOrderByItemsWhenColumnMatchesDifferentProjectionAliasIgnoringCase() {
246+
ProjectionsSegment projectionsSegment = new ProjectionsSegment(0, 0);
247+
ColumnSegmentBoundInfo boundInfo = new ColumnSegmentBoundInfo(new IdentifierValue("name"));
248+
ColumnSegment projectionColumn = new ColumnSegment(0, 0, new IdentifierValue("name"));
249+
projectionColumn.setColumnBoundInfo(boundInfo);
250+
ColumnProjectionSegment projectionSegment = new ColumnProjectionSegment(projectionColumn);
251+
projectionSegment.setAlias(new AliasSegment(0, 0, new IdentifierValue("ID")));
252+
projectionsSegment.getProjections().add(projectionSegment);
253+
ColumnSegment groupByColumn = new ColumnSegment(0, 0, new IdentifierValue("id"));
254+
groupByColumn.setColumnBoundInfo(boundInfo);
255+
ColumnSegment orderByColumn = new ColumnSegment(0, 0, new IdentifierValue("id"));
256+
orderByColumn.setColumnBoundInfo(boundInfo);
257+
SelectStatement selectStatement = SelectStatement.builder().databaseType(databaseType).projections(projectionsSegment)
258+
.groupBy(new GroupBySegment(0, 0, Collections.singletonList(
259+
new ColumnOrderByItemSegment(groupByColumn, OrderDirection.ASC, NullsOrderType.LAST))))
260+
.orderBy(new OrderBySegment(0, 0, Collections.singletonList(
261+
new ColumnOrderByItemSegment(orderByColumn, OrderDirection.DESC, NullsOrderType.LAST))))
262+
.build();
263+
SelectStatementContext selectStatementContext = createSelectStatementContext(selectStatement);
264+
assertFalse(selectStatementContext.isSameGroupByAndOrderByItems());
265+
}
194266

195267
@Test
196268
void assertIsNotSameGroupByAndOrderByItemsWhenEmptyGroupBy() {

‎test/it/rewriter/src/test/resources/scenario/sharding/case/dml/select.xml‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -291,14 +291,14 @@
291291

292292
<rewrite-assertion id="select_limit_with_multiple_route_with_memory_group_by_for_parameters_for_mysql" db-types="MySQL">
293293
<input sql="SELECT * FROM t_account WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ?, ?" parameters="100, 10" />
294-
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ?, ?" parameters="0, 2147483647" />
295-
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ?, ?" parameters="0, 2147483647" />
294+
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ?, ?" parameters="0, 110" />
295+
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ?, ?" parameters="0, 110" />
296296
</rewrite-assertion>
297297

298298
<rewrite-assertion id="select_limit_with_multiple_route_with_memory_group_by_for_literals_for_mysql" db-types="MySQL">
299299
<input sql="SELECT * FROM t_account WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 100, 10" />
300-
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 0, 2147483647" />
301-
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 0, 2147483647" />
300+
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 0, 110" />
301+
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 0, 110" />
302302
</rewrite-assertion>
303303

304304
<rewrite-assertion id="select_limit_with_single_route_for_parameters_for_postgresql" db-types="PostgreSQL,openGauss">
@@ -325,14 +325,14 @@
325325

326326
<rewrite-assertion id="select_limit_with_multiple_route_with_memory_group_by_for_parameters_for_postgresql" db-types="PostgreSQL,openGauss">
327327
<input sql="SELECT * FROM t_account WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ? OFFSET ?" parameters="10, 100" />
328-
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ? OFFSET ?" parameters="2147483647, 0" />
329-
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ? OFFSET ?" parameters="2147483647, 0" />
328+
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ? OFFSET ?" parameters="110, 0" />
329+
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT ? OFFSET ?" parameters="110, 0" />
330330
</rewrite-assertion>
331331

332332
<rewrite-assertion id="select_limit_with_multiple_route_with_memory_group_by_for_literals_for_postgresql" db-types="PostgreSQL,openGauss">
333333
<input sql="SELECT * FROM t_account WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 10 OFFSET 100" />
334-
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 2147483647 OFFSET 0" />
335-
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 2147483647 OFFSET 0" />
334+
<output sql="SELECT * FROM t_account_0 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 110 OFFSET 0" />
335+
<output sql="SELECT * FROM t_account_1 WHERE account_id IN (100, 101) GROUP BY account_id ORDER BY account_id DESC LIMIT 110 OFFSET 0" />
336336
</rewrite-assertion>
337337

338338
<!-- FIXME -->

0 commit comments

Comments
 (0)