You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/01/22 11:33:14 UTC
[GitHub] [shardingsphere] sandynz opened a new pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
sandynz opened a new pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133
Fixes #9109.
Changes proposed in this pull request:
- Order by primary keys ascending if simple query sql without order by clause for MySQL (just support simple query sql: single table, no join tables, no subquery, no aggregation, table has primary key)
----------------------------------------------------------------
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] [shardingsphere] terrymanu commented on pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
terrymanu commented on pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#issuecomment-766746069
> Blocked on integration test MySQL-Proxy, aggregation function with `DISTINCT` test failed on `Placeholder` mode, e.g.
>
> ```
> Error case: assertExecuteQuery[proxy: db -> MySQL -> Placeholder -> SELECT SUM(DISTINCT order_id) s FROM t_order WHERE order_id < 1100]
>
> java.lang.AssertionError: Size of actual result set is different with size of expected dat set rows.
> Expected: is <1>
> but: was <0>
>
> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
> at org.junit.Assert.assertThat(Assert.java:956)
> at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertRows(BaseDQLIT.java:102)
> at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertResultSet(BaseDQLIT.java:71)
> at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQueryForPreparedStatement(GeneralDQLIT.java:81)
> at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQuery(GeneralDQLIT.java:62)
> ```
We can ignore that case because it is known case, I will fix it soon
----------------------------------------------------------------
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] [shardingsphere] terrymanu merged pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133
----------------------------------------------------------------
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] [shardingsphere] sandynz commented on pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#issuecomment-766562047
Blocked on integration test MySQL-Proxy, aggregation function with `DISTINCT` test failed on `Placeholder` mode, e.g.
```
Error case: assertExecuteQuery[proxy: db -> MySQL -> Placeholder -> SELECT SUM(DISTINCT order_id) s FROM t_order WHERE order_id < 1100]
java.lang.AssertionError: Size of actual result set is different with size of expected dat set rows.
Expected: is <1>
but: was <0>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.junit.Assert.assertThat(Assert.java:956)
at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertRows(BaseDQLIT.java:102)
at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertResultSet(BaseDQLIT.java:71)
at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQueryForPreparedStatement(GeneralDQLIT.java:81)
at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQuery(GeneralDQLIT.java:62)
```
----------------------------------------------------------------
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] [shardingsphere] terrymanu commented on a change in pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#discussion_r565022616
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/orderby/engine/OrderByContextEngine.java
##########
@@ -78,4 +96,42 @@ private OrderByContext createOrderByContextForDistinctRowWithoutGroupBy(final Se
}
return null;
}
+
+ private OrderByContext createOrderByContextForMySQLSelectWithoutOrderBy(final ShardingSphereSchema schema, final SelectStatement selectStatement, final GroupByContext groupByContext) {
+ if (groupByContext.getItems().size() > 0) {
Review comment:
`groupByContext.getItems().size() > 0` can be replace to `!groupByContext.getItems().isEmpty()`
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/orderby/engine/OrderByContextEngine.java
##########
@@ -78,4 +96,42 @@ private OrderByContext createOrderByContextForDistinctRowWithoutGroupBy(final Se
}
return null;
}
+
+ private OrderByContext createOrderByContextForMySQLSelectWithoutOrderBy(final ShardingSphereSchema schema, final SelectStatement selectStatement, final GroupByContext groupByContext) {
Review comment:
It is better to refactor return null to return `Optional`
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/src/main/java/org/apache/shardingsphere/infra/binder/segment/select/orderby/engine/OrderByContextEngine.java
##########
@@ -78,4 +96,42 @@ private OrderByContext createOrderByContextForDistinctRowWithoutGroupBy(final Se
}
return null;
}
+
+ private OrderByContext createOrderByContextForMySQLSelectWithoutOrderBy(final ShardingSphereSchema schema, final SelectStatement selectStatement, final GroupByContext groupByContext) {
+ if (groupByContext.getItems().size() > 0) {
+ return null;
+ }
+
+ TableSegment tableSegment = selectStatement.getFrom();
+ if (null == tableSegment) {
+ return null;
+ }
+ if (!(tableSegment instanceof SimpleTableSegment)) {
+ return null;
+ }
+ String tableName = ((SimpleTableSegment) tableSegment).getTableName().getIdentifier().getValue();
+ TableMetaData tableMetaData = schema.get(tableName);
+ if (null == tableMetaData) {
+ return null;
+ }
+
+ for (ProjectionSegment projectionSegment : selectStatement.getProjections().getProjections()) {
+ if (projectionSegment instanceof AggregationProjectionSegment) {
+ return null;
+ }
+ }
+
+ int index = 0;
+ List<OrderByItem> orderByItems = new LinkedList<>();
+ for (String primaryKeyColumn : tableMetaData.getPrimaryKeyColumns()) {
Review comment:
`primaryKeyColumn` should rename to `each`
----------------------------------------------------------------
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] [shardingsphere] terrymanu commented on a change in pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#discussion_r562692852
##########
File path: shardingsphere-infra/shardingsphere-infra-binder/pom.xml
##########
@@ -43,5 +43,12 @@
<artifactId>shardingsphere-infra-common</artifactId>
<version>${project.version}</version>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.shardingsphere</groupId>
+ <artifactId>shardingsphere-sql-parser-mysql</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
The infra module can not understand concrete module such as shardingsphere-sql-parser-mysql
----------------------------------------------------------------
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] [shardingsphere] sandynz commented on pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
sandynz commented on pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#issuecomment-766562047
Blocked on integration test MySQL-Proxy, aggregation function with `DISTINCT` test failed on `Placeholder` mode, e.g.
```
Error case: assertExecuteQuery[proxy: db -> MySQL -> Placeholder -> SELECT SUM(DISTINCT order_id) s FROM t_order WHERE order_id < 1100]
java.lang.AssertionError: Size of actual result set is different with size of expected dat set rows.
Expected: is <1>
but: was <0>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.junit.Assert.assertThat(Assert.java:956)
at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertRows(BaseDQLIT.java:102)
at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertResultSet(BaseDQLIT.java:71)
at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQueryForPreparedStatement(GeneralDQLIT.java:81)
at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQuery(GeneralDQLIT.java:62)
```
----------------------------------------------------------------
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] [shardingsphere] terrymanu commented on pull request #9133: Order by primary keys ascending if simple query sql without order by clause for MySQL
Posted by GitBox <gi...@apache.org>.
terrymanu commented on pull request #9133:
URL: https://github.com/apache/shardingsphere/pull/9133#issuecomment-766746069
> Blocked on integration test MySQL-Proxy, aggregation function with `DISTINCT` test failed on `Placeholder` mode, e.g.
>
> ```
> Error case: assertExecuteQuery[proxy: db -> MySQL -> Placeholder -> SELECT SUM(DISTINCT order_id) s FROM t_order WHERE order_id < 1100]
>
> java.lang.AssertionError: Size of actual result set is different with size of expected dat set rows.
> Expected: is <1>
> but: was <0>
>
> at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
> at org.junit.Assert.assertThat(Assert.java:956)
> at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertRows(BaseDQLIT.java:102)
> at org.apache.shardingsphere.test.integration.engine.it.dql.BaseDQLIT.assertResultSet(BaseDQLIT.java:71)
> at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQueryForPreparedStatement(GeneralDQLIT.java:81)
> at org.apache.shardingsphere.test.integration.engine.it.dql.GeneralDQLIT.assertExecuteQuery(GeneralDQLIT.java:62)
> ```
We can ignore that case because it is known case, I will fix it soon
----------------------------------------------------------------
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