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 2020/09/16 06:28:41 UTC
[GitHub] [shardingsphere] jingshanglu opened a new pull request #7476: Rewrite derived projection from orderby
jingshanglu opened a new pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476
Fixes #5857.
Changes proposed in this pull request:
- fix rewrite derived projection from orderby
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489279727
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -44,17 +60,26 @@ public boolean isGenerateSQLToken(final SQLStatementContext sqlStatementContext)
@Override
public ProjectionsToken generateSQLToken(final SelectStatementContext selectStatementContext) {
- Collection<String> derivedProjectionTexts = getDerivedProjectionTexts(selectStatementContext);
+ Map<RouteUnit, Collection<String>> derivedProjectionTexts = getDerivedProjectionTexts(selectStatementContext);
return new ProjectionsToken(selectStatementContext.getProjectionsContext().getStopIndex() + 1 + " ".length(), derivedProjectionTexts);
}
- private Collection<String> getDerivedProjectionTexts(final SelectStatementContext selectStatementContext) {
- Collection<String> result = new LinkedList<>();
- for (Projection each : selectStatementContext.getProjectionsContext().getProjections()) {
- if (each instanceof AggregationProjection && !((AggregationProjection) each).getDerivedAggregationProjections().isEmpty()) {
- result.addAll(((AggregationProjection) each).getDerivedAggregationProjections().stream().map(this::getDerivedProjectionText).collect(Collectors.toList()));
- } else if (each instanceof DerivedProjection) {
- result.add(getDerivedProjectionText(each));
+ private Map<RouteUnit, Collection<String>> getDerivedProjectionTexts(final SelectStatementContext selectStatementContext) {
+ Map<RouteUnit, Collection<String>> result = new HashMap<>();
+ for (RouteUnit routeUnit : routeContext.getRouteResult().getRouteUnits()) {
+ result.put(routeUnit, new LinkedList<>());
+ for (Projection each : selectStatementContext.getProjectionsContext().getProjections()) {
+ if (each instanceof AggregationProjection && !((AggregationProjection) each).getDerivedAggregationProjections().isEmpty()) {
+ result.get(routeUnit).addAll(((AggregationProjection) each).getDerivedAggregationProjections().stream().map(this::getDerivedProjectionText).collect(Collectors.toList()));
+ } else if (each instanceof DerivedProjection) {
+ if (!(((DerivedProjection) each).getRealProjection() instanceof ColumnOrderByItemSegment)) {
+ result.get(routeUnit).add(getDerivedProjectionText(each));
+ continue;
+ }
+ TableExtractUtils utils = new TableExtractUtils();
+ utils.extractTablesFromSelect(selectStatementContext.getSqlStatement());
+ result.get(routeUnit).add(getDerivedProjectionTextFromColumnOrderByItemSegment((DerivedProjection) each, utils, routeUnit));
+ }
Review comment:
The code's cyclomatic complexity is pretty high, it is better to extract serval private method for nested loop and if statement
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -67,4 +92,40 @@ private String getDerivedProjectionText(final Projection projection) {
}
return projection.getExpression() + " AS " + projection.getAlias().get() + " ";
}
+
+ private String getDerivedProjectionTextFromColumnOrderByItemSegment(final DerivedProjection projection, final TableExtractUtils utils, final RouteUnit routeUnit) {
+ Preconditions.checkState(projection.getAlias().isPresent());
+ Preconditions.checkState(projection.getRealProjection() instanceof ColumnOrderByItemSegment);
+ ColumnOrderByItemSegment columnOrderByItemSegment = (ColumnOrderByItemSegment) projection.getRealProjection();
+ ColumnOrderByItemSegment newColumnOrderByItem = generateNewColumnOrderByItem(columnOrderByItemSegment, routeUnit, utils);
+ return newColumnOrderByItem.getText() + " AS " + projection.getAlias().get() + " ";
+ }
+
+ private Optional<String> getActualTables(final RouteUnit routeUnit, final String logicalTableName) {
+ for (RouteMapper each : routeUnit.getTableMappers()) {
+ if (each.getLogicName().equalsIgnoreCase(logicalTableName)) {
+ return Optional.of(each.getActualName());
+ }
+ }
+ return Optional.empty();
+ }
+
+ private ColumnOrderByItemSegment generateNewColumnOrderByItem(final ColumnOrderByItemSegment old, final RouteUnit routeUnit, final TableExtractUtils utils) {
+ Optional<OwnerSegment> ownerSegment = old.getColumn().getOwner();
+ if (!ownerSegment.isPresent()) {
+ return old;
+ }
+ if (!utils.needRewrite(ownerSegment.get())) {
+ return old;
+ }
+ Optional<String> actualTableName = getActualTables(routeUnit, ownerSegment.get().getIdentifier().getValue());
+ Preconditions.checkState(actualTableName.isPresent());
+ ColumnSegment newColumnSegment = new ColumnSegment(0, 0, old.getColumn().getIdentifier());
+ IdentifierValue newOwnerIdentifier = new IdentifierValue(ownerSegment.get().getIdentifier().getQuoteCharacter().getStartDelimiter()
+ + actualTableName.get() + ownerSegment.get().getIdentifier().getQuoteCharacter().getEndDelimiter());
+ OwnerSegment newOwner = new OwnerSegment(0, 0, newOwnerIdentifier);
+ newColumnSegment.setOwner(newOwner);
+ ColumnOrderByItemSegment newColumnOrderByItem = new ColumnOrderByItemSegment(newColumnSegment, old.getOrderDirection());
+ return newColumnOrderByItem;
Review comment:
The return value should be newColumnOrderByItem
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -67,4 +92,40 @@ private String getDerivedProjectionText(final Projection projection) {
}
return projection.getExpression() + " AS " + projection.getAlias().get() + " ";
}
+
+ private String getDerivedProjectionTextFromColumnOrderByItemSegment(final DerivedProjection projection, final TableExtractUtils utils, final RouteUnit routeUnit) {
+ Preconditions.checkState(projection.getAlias().isPresent());
+ Preconditions.checkState(projection.getRealProjection() instanceof ColumnOrderByItemSegment);
+ ColumnOrderByItemSegment columnOrderByItemSegment = (ColumnOrderByItemSegment) projection.getRealProjection();
+ ColumnOrderByItemSegment newColumnOrderByItem = generateNewColumnOrderByItem(columnOrderByItemSegment, routeUnit, utils);
+ return newColumnOrderByItem.getText() + " AS " + projection.getAlias().get() + " ";
+ }
+
+ private Optional<String> getActualTables(final RouteUnit routeUnit, final String logicalTableName) {
+ for (RouteMapper each : routeUnit.getTableMappers()) {
+ if (each.getLogicName().equalsIgnoreCase(logicalTableName)) {
+ return Optional.of(each.getActualName());
+ }
+ }
+ return Optional.empty();
+ }
+
+ private ColumnOrderByItemSegment generateNewColumnOrderByItem(final ColumnOrderByItemSegment old, final RouteUnit routeUnit, final TableExtractUtils utils) {
+ Optional<OwnerSegment> ownerSegment = old.getColumn().getOwner();
+ if (!ownerSegment.isPresent()) {
+ return old;
+ }
+ if (!utils.needRewrite(ownerSegment.get())) {
+ return old;
+ }
+ Optional<String> actualTableName = getActualTables(routeUnit, ownerSegment.get().getIdentifier().getValue());
+ Preconditions.checkState(actualTableName.isPresent());
+ ColumnSegment newColumnSegment = new ColumnSegment(0, 0, old.getColumn().getIdentifier());
+ IdentifierValue newOwnerIdentifier = new IdentifierValue(ownerSegment.get().getIdentifier().getQuoteCharacter().getStartDelimiter()
Review comment:
String with `+` is not good practice, it is better to replace to String.format or StringBuilder
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ProjectionsToken.java
##########
@@ -18,26 +18,29 @@
package org.apache.shardingsphere.sharding.rewrite.token.pojo;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.Attachable;
+import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.RouteUnitAware;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.SQLToken;
+import org.apache.shardingsphere.infra.route.context.RouteUnit;
import java.util.Collection;
+import java.util.Map;
/**
* Projections token.
*/
-public final class ProjectionsToken extends SQLToken implements Attachable {
+public final class ProjectionsToken extends SQLToken implements Attachable, RouteUnitAware {
- private final Collection<String> projections;
+ private final Map<RouteUnit, Collection<String>> projections;
- public ProjectionsToken(final int startIndex, final Collection<String> projections) {
+ public ProjectionsToken(final int startIndex, final Map<RouteUnit, Collection<String>> projections) {
Review comment:
The object Map<RouteUnit, Collection<String>> it too hard to understand, can you consider about to create a new object to describe it?
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractUtils.java
##########
@@ -198,7 +198,12 @@ public void extractTablesFromUpdate(final UpdateStatement updateStatement) {
}
}
- private boolean needRewrite(final OwnerSegment owner) {
+ /**
Review comment:
Util class should be final and static
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/segment/select/projection/impl/DerivedProjection.java
##########
@@ -38,6 +39,8 @@
private final String alias;
+ private final SQLSegment realProjection;
Review comment:
what mean of realProjection? For my understanding, derived projection do not have `real projection`, can you rename it as correct name?
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489301820
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ProjectionsToken.java
##########
@@ -18,26 +18,29 @@
package org.apache.shardingsphere.sharding.rewrite.token.pojo;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.Attachable;
+import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.RouteUnitAware;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.SQLToken;
+import org.apache.shardingsphere.infra.route.context.RouteUnit;
import java.util.Collection;
+import java.util.Map;
/**
* Projections token.
*/
-public final class ProjectionsToken extends SQLToken implements Attachable {
+public final class ProjectionsToken extends SQLToken implements Attachable, RouteUnitAware {
- private final Collection<String> projections;
+ private final Map<RouteUnit, Collection<String>> projections;
- public ProjectionsToken(final int startIndex, final Collection<String> projections) {
+ public ProjectionsToken(final int startIndex, final Map<RouteUnit, Collection<String>> projections) {
Review comment:
The struct just a result for all RouteUnit, different routeUnit correspond to different rewrited projection, if create a new object,it maybe a little heavy.
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491658545
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
I think it is just a util class with status, what package do you think is more appropriate?
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476
----------------------------------------------------------------
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] coveralls edited a comment on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14872](https://coveralls.io/builds/33550683)
* **63** of **70** **(90.0%)** changed or added relevant lines in **9** files are covered.
* **13699** unchanged lines in **39** files lost coverage.
* Overall coverage increased (+**0.03%**) to **35.083%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L79) | 38 | 41 | 92.68%
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Futil%2FTableExtractor.java#L49) | 0 | 4 | 0.0%
<!-- | **Total:** | **63** | **70** | **90.0%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-features/shardingsphere-encrypt/shardingsphere-encrypt-rewrite/src/main/java/org/apache/shardingsphere/encrypt/rewrite/token/generator/impl/EncryptPredicateColumnTokenGenerator.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-features%2Fshardingsphere-encrypt%2Fshardingsphere-encrypt-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fencrypt%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FEncryptPredicateColumnTokenGenerator.java#L75) | 1 | 96.97% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/config/YamlShadowRuleConfiguration.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fconfig%2FYamlShadowRuleConfiguration.java#L42) | 1 | 50.0% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/generic/MySQLUnsupportedCommandExecutor.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2Fcommand%2Fgeneric%2FMySQLUnsupportedCommandExecutor.java#L38) | 1 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/spi/ScalingEntryLoader.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Fspi%2FScalingEntryLoader.java#L51) | 1 | 88.89% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/utils/ThreadUtil.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Futils%2FThreadUtil.java#L37) | 1 | 75.0% |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-common/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/inline/InlineShardingAlgorithm.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Falgorithm%2Fsharding%2Finline%2FInlineShardingAlgorithm.java#L77) | 2 | 88.89% |
| [shardingsphere-governance/shardingsphere-governance-core/shardingsphere-governance-core-config/src/main/java/org/apache/shardingsphere/governance/core/config/ConfigCenter.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-governance%2Fshardingsphere-governance-core%2Fshardingsphere-governance-core-config%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fgovernance%2Fcore%2Fconfig%2FConfigCenter.java#L216) | 2 | 89.92% |
| [shardingsphere-proxy/shardingsphere-proxy-bootstrap/src/main/java/org/apache/shardingsphere/proxy/Bootstrap.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-bootstrap%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2FBootstrap.java#L46) | 2 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-mysql/src/main/java/org/apache/shardingsphere/scaling/mysql/MySQLPositionManager.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fmysql%2FMySQLPositionManager.java#L33) | 2 | 93.55% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/DeleteStatement.java](https://coveralls.io/builds/33550683/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Fstatement%2Fdml%2FDeleteStatement.java#L33) | 2 | 0% |
<!-- | **Total:** | **13699** | | -->
| Totals | [](https://coveralls.io/builds/33550683) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.03% |
| Covered Lines: | 36585 |
| Relevant Lines: | 104280 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r490643195
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/statement/dml/UpdateStatementContext.java
##########
@@ -42,16 +42,16 @@
public UpdateStatementContext(final UpdateStatement sqlStatement) {
super(sqlStatement);
- TableExtractUtils utils = new TableExtractUtils();
+ TableExtractor utils = new TableExtractor();
Review comment:
Ok
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491043925
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
Is the class TableExtractor suitable in util package?
if it is not an util class, please move it to correct package.
----------------------------------------------------------------
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] coveralls commented on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14819](https://coveralls.io/builds/33494843)
* **39** of **43** **(90.7%)** changed or added relevant lines in **4** files are covered.
* **13437** unchanged lines in **2** files lost coverage.
* Overall coverage increased (+**0.02%**) to **35.068%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33494843/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L76) | 35 | 39 | 89.74%
<!-- | **Total:** | **39** | **43** | **90.7%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementBaseVisitor.java](https://coveralls.io/builds/33494843/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementBaseVisitor.java#L811) | 18 | 2.16% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementParser.java](https://coveralls.io/builds/33494843/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementParser.java#L652) | 13419 | 18.06% |
<!-- | **Total:** | **13437** | | -->
| Totals | [](https://coveralls.io/builds/33494843) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.02% |
| Covered Lines: | 36571 |
| Relevant Lines: | 104287 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491919555
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
Ok
----------------------------------------------------------------
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] coveralls edited a comment on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14859](https://coveralls.io/builds/33522491)
* **59** of **64** **(92.19%)** changed or added relevant lines in **9** files are covered.
* **13698** unchanged lines in **38** files lost coverage.
* Overall coverage increased (+**0.02%**) to **35.073%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Futil%2FTableExtractor.java#L49) | 0 | 2 | 0.0%
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L79) | 38 | 41 | 92.68%
<!-- | **Total:** | **59** | **64** | **92.19%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-features/shardingsphere-encrypt/shardingsphere-encrypt-rewrite/src/main/java/org/apache/shardingsphere/encrypt/rewrite/token/generator/impl/EncryptPredicateColumnTokenGenerator.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-features%2Fshardingsphere-encrypt%2Fshardingsphere-encrypt-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fencrypt%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FEncryptPredicateColumnTokenGenerator.java#L75) | 1 | 96.97% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/config/YamlShadowRuleConfiguration.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fconfig%2FYamlShadowRuleConfiguration.java#L42) | 1 | 50.0% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/generic/MySQLUnsupportedCommandExecutor.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2Fcommand%2Fgeneric%2FMySQLUnsupportedCommandExecutor.java#L38) | 1 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/spi/ScalingEntryLoader.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Fspi%2FScalingEntryLoader.java#L51) | 1 | 88.89% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/utils/ThreadUtil.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Futils%2FThreadUtil.java#L37) | 1 | 75.0% |
| [shardingsphere-governance/shardingsphere-governance-core/shardingsphere-governance-core-config/src/main/java/org/apache/shardingsphere/governance/core/config/ConfigCenter.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-governance%2Fshardingsphere-governance-core%2Fshardingsphere-governance-core-config%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fgovernance%2Fcore%2Fconfig%2FConfigCenter.java#L216) | 2 | 89.92% |
| [shardingsphere-proxy/shardingsphere-proxy-bootstrap/src/main/java/org/apache/shardingsphere/proxy/Bootstrap.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-bootstrap%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2FBootstrap.java#L46) | 2 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-mysql/src/main/java/org/apache/shardingsphere/scaling/mysql/MySQLPositionManager.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fmysql%2FMySQLPositionManager.java#L33) | 2 | 93.55% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/DeleteStatement.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Fstatement%2Fdml%2FDeleteStatement.java#L33) | 2 | 0% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/swapper/ShadowRuleConfigurationYamlSwapper.java](https://coveralls.io/builds/33522491/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fswapper%2FShadowRuleConfigurationYamlSwapper.java#L41) | 3 | 70.0% |
<!-- | **Total:** | **13698** | | -->
| Totals | [](https://coveralls.io/builds/33522491) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.02% |
| Covered Lines: | 36574 |
| Relevant Lines: | 104280 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489306602
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -44,17 +60,26 @@ public boolean isGenerateSQLToken(final SQLStatementContext sqlStatementContext)
@Override
public ProjectionsToken generateSQLToken(final SelectStatementContext selectStatementContext) {
- Collection<String> derivedProjectionTexts = getDerivedProjectionTexts(selectStatementContext);
+ Map<RouteUnit, Collection<String>> derivedProjectionTexts = getDerivedProjectionTexts(selectStatementContext);
return new ProjectionsToken(selectStatementContext.getProjectionsContext().getStopIndex() + 1 + " ".length(), derivedProjectionTexts);
}
- private Collection<String> getDerivedProjectionTexts(final SelectStatementContext selectStatementContext) {
- Collection<String> result = new LinkedList<>();
- for (Projection each : selectStatementContext.getProjectionsContext().getProjections()) {
- if (each instanceof AggregationProjection && !((AggregationProjection) each).getDerivedAggregationProjections().isEmpty()) {
- result.addAll(((AggregationProjection) each).getDerivedAggregationProjections().stream().map(this::getDerivedProjectionText).collect(Collectors.toList()));
- } else if (each instanceof DerivedProjection) {
- result.add(getDerivedProjectionText(each));
+ private Map<RouteUnit, Collection<String>> getDerivedProjectionTexts(final SelectStatementContext selectStatementContext) {
+ Map<RouteUnit, Collection<String>> result = new HashMap<>();
+ for (RouteUnit routeUnit : routeContext.getRouteResult().getRouteUnits()) {
+ result.put(routeUnit, new LinkedList<>());
+ for (Projection each : selectStatementContext.getProjectionsContext().getProjections()) {
+ if (each instanceof AggregationProjection && !((AggregationProjection) each).getDerivedAggregationProjections().isEmpty()) {
+ result.get(routeUnit).addAll(((AggregationProjection) each).getDerivedAggregationProjections().stream().map(this::getDerivedProjectionText).collect(Collectors.toList()));
+ } else if (each instanceof DerivedProjection) {
+ if (!(((DerivedProjection) each).getRealProjection() instanceof ColumnOrderByItemSegment)) {
+ result.get(routeUnit).add(getDerivedProjectionText(each));
+ continue;
+ }
+ TableExtractUtils utils = new TableExtractUtils();
+ utils.extractTablesFromSelect(selectStatementContext.getSqlStatement());
+ result.get(routeUnit).add(getDerivedProjectionTextFromColumnOrderByItemSegment((DerivedProjection) each, utils, routeUnit));
+ }
Review comment:
Ok
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489415698
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -67,4 +90,41 @@ private String getDerivedProjectionText(final Projection projection) {
}
return projection.getExpression() + " AS " + projection.getAlias().get() + " ";
}
+
+ private String getDerivedProjectionTextFromColumnOrderByItemSegment(final DerivedProjection projection, final TableExtractUtils utils, final RouteUnit routeUnit) {
+ Preconditions.checkState(projection.getAlias().isPresent());
+ Preconditions.checkState(projection.getProjection() instanceof ColumnOrderByItemSegment);
+ ColumnOrderByItemSegment columnOrderByItemSegment = (ColumnOrderByItemSegment) projection.getProjection();
+ ColumnOrderByItemSegment newColumnOrderByItem = generateNewColumnOrderByItem(columnOrderByItemSegment, routeUnit, utils);
+ return newColumnOrderByItem.getText() + " AS " + projection.getAlias().get() + " ";
Review comment:
How about to avoid using `+` with string
----------------------------------------------------------------
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] coveralls edited a comment on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14827](https://coveralls.io/builds/33497503)
* **40** of **44** **(90.91%)** changed or added relevant lines in **4** files are covered.
* **13448** unchanged lines in **6** files lost coverage.
* Overall coverage increased (+**0.02%**) to **35.065%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L76) | 36 | 40 | 90.0%
<!-- | **Total:** | **40** | **44** | **90.91%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/config/YamlShadowRuleConfiguration.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fconfig%2FYamlShadowRuleConfiguration.java#L42) | 1 | 50.0% |
| [shardingsphere-governance/shardingsphere-governance-core/shardingsphere-governance-core-config/src/main/java/org/apache/shardingsphere/governance/core/config/ConfigCenter.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-governance%2Fshardingsphere-governance-core%2Fshardingsphere-governance-core-config%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fgovernance%2Fcore%2Fconfig%2FConfigCenter.java#L216) | 2 | 89.92% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/swapper/ShadowRuleConfigurationYamlSwapper.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fswapper%2FShadowRuleConfigurationYamlSwapper.java#L41) | 3 | 70.0% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-api/src/main/java/org/apache/shardingsphere/shadow/api/config/ShadowRuleConfiguration.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fapi%2Fconfig%2FShadowRuleConfiguration.java#L43) | 5 | 0% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementBaseVisitor.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementBaseVisitor.java#L811) | 18 | 2.16% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementParser.java](https://coveralls.io/builds/33497503/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementParser.java#L652) | 13419 | 18.06% |
<!-- | **Total:** | **13448** | | -->
| Totals | [](https://coveralls.io/builds/33497503) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.02% |
| Covered Lines: | 36568 |
| Relevant Lines: | 104286 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r490420285
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/statement/dml/UpdateStatementContext.java
##########
@@ -42,16 +42,16 @@
public UpdateStatementContext(final UpdateStatement sqlStatement) {
super(sqlStatement);
- TableExtractUtils utils = new TableExtractUtils();
+ TableExtractor utils = new TableExtractor();
Review comment:
Please rename variable name `utils` because of there is no any relationship with variable name and class type.
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489227432
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -26,16 +32,26 @@
import org.apache.shardingsphere.sql.parser.binder.segment.select.projection.impl.DerivedProjection;
import org.apache.shardingsphere.sql.parser.binder.statement.SQLStatementContext;
import org.apache.shardingsphere.sql.parser.binder.statement.dml.SelectStatementContext;
-import org.apache.shardingsphere.infra.rewrite.sql.token.generator.OptionalSQLTokenGenerator;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.dml.column.ColumnSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.dml.order.item.ColumnOrderByItemSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.generic.OwnerSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.util.TableExtractUtils;
+import org.apache.shardingsphere.sql.parser.sql.common.value.identifier.IdentifierValue;
import java.util.Collection;
+import java.util.HashMap;
import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
import java.util.stream.Collectors;
/**
* Projections token generator.
*/
-public final class ProjectionsTokenGenerator implements OptionalSQLTokenGenerator<SelectStatementContext>, IgnoreForSingleRoute {
+public final class ProjectionsTokenGenerator implements OptionalSQLTokenGenerator<SelectStatementContext>, IgnoreForSingleRoute, RouteContextAware {
+
+ @Setter
+ private RouteContext routeContext;
Review comment:
Any reason to add setter routeContext here?
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489850128
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractUtils.java
##########
@@ -198,7 +198,12 @@ public void extractTablesFromUpdate(final UpdateStatement updateStatement) {
}
}
- private boolean needRewrite(final OwnerSegment owner) {
+ /**
Review comment:
Ok,I'll change the class name.
----------------------------------------------------------------
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] coveralls edited a comment on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14858](https://coveralls.io/builds/33521825)
* **59** of **64** **(92.19%)** changed or added relevant lines in **9** files are covered.
* **13698** unchanged lines in **38** files lost coverage.
* Overall coverage increased (+**0.02%**) to **35.073%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Futil%2FTableExtractor.java#L49) | 0 | 2 | 0.0%
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L79) | 38 | 41 | 92.68%
<!-- | **Total:** | **59** | **64** | **92.19%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-features/shardingsphere-encrypt/shardingsphere-encrypt-rewrite/src/main/java/org/apache/shardingsphere/encrypt/rewrite/token/generator/impl/EncryptPredicateColumnTokenGenerator.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-features%2Fshardingsphere-encrypt%2Fshardingsphere-encrypt-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fencrypt%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FEncryptPredicateColumnTokenGenerator.java#L75) | 1 | 96.97% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/config/YamlShadowRuleConfiguration.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fconfig%2FYamlShadowRuleConfiguration.java#L42) | 1 | 50.0% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/generic/MySQLUnsupportedCommandExecutor.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2Fcommand%2Fgeneric%2FMySQLUnsupportedCommandExecutor.java#L38) | 1 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/spi/ScalingEntryLoader.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Fspi%2FScalingEntryLoader.java#L51) | 1 | 88.89% |
| [shardingsphere-scaling/shardingsphere-scaling-core/src/main/java/org/apache/shardingsphere/scaling/core/utils/ThreadUtil.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fcore%2Futils%2FThreadUtil.java#L37) | 1 | 75.0% |
| [shardingsphere-governance/shardingsphere-governance-core/shardingsphere-governance-core-config/src/main/java/org/apache/shardingsphere/governance/core/config/ConfigCenter.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-governance%2Fshardingsphere-governance-core%2Fshardingsphere-governance-core-config%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fgovernance%2Fcore%2Fconfig%2FConfigCenter.java#L216) | 2 | 89.92% |
| [shardingsphere-proxy/shardingsphere-proxy-bootstrap/src/main/java/org/apache/shardingsphere/proxy/Bootstrap.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-bootstrap%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2FBootstrap.java#L46) | 2 | 0% |
| [shardingsphere-scaling/shardingsphere-scaling-mysql/src/main/java/org/apache/shardingsphere/scaling/mysql/MySQLPositionManager.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-scaling%2Fshardingsphere-scaling-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fscaling%2Fmysql%2FMySQLPositionManager.java#L33) | 2 | 93.55% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/DeleteStatement.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fcommon%2Fstatement%2Fdml%2FDeleteStatement.java#L33) | 2 | 0% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/swapper/ShadowRuleConfigurationYamlSwapper.java](https://coveralls.io/builds/33521825/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fswapper%2FShadowRuleConfigurationYamlSwapper.java#L41) | 3 | 70.0% |
<!-- | **Total:** | **13698** | | -->
| Totals | [](https://coveralls.io/builds/33521825) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.02% |
| Covered Lines: | 36574 |
| Relevant Lines: | 104280 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489417167
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/segment/select/projection/impl/DerivedProjection.java
##########
@@ -38,6 +39,8 @@
private final String alias;
+ private final SQLSegment projection;
Review comment:
original projection maybe a better name
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ProjectionsToken.java
##########
@@ -18,26 +18,29 @@
package org.apache.shardingsphere.sharding.rewrite.token.pojo;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.Attachable;
+import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.RouteUnitAware;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.SQLToken;
+import org.apache.shardingsphere.infra.route.context.RouteUnit;
import java.util.Collection;
+import java.util.Map;
/**
* Projections token.
*/
-public final class ProjectionsToken extends SQLToken implements Attachable {
+public final class ProjectionsToken extends SQLToken implements Attachable, RouteUnitAware {
- private final Collection<String> projections;
+ private final Map<RouteUnit, Collection<String>> projections;
- public ProjectionsToken(final int startIndex, final Collection<String> projections) {
+ public ProjectionsToken(final int startIndex, final Map<RouteUnit, Collection<String>> projections) {
Review comment:
t too hard to understand, what is the Collection<String>?
It is better to make the code for read easier.
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractUtils.java
##########
@@ -198,7 +198,12 @@ public void extractTablesFromUpdate(final UpdateStatement updateStatement) {
}
}
- private boolean needRewrite(final OwnerSegment owner) {
+ /**
Review comment:
It is invalid if util class has state, please consider change class name
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489904704
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ProjectionsToken.java
##########
@@ -18,26 +18,29 @@
package org.apache.shardingsphere.sharding.rewrite.token.pojo;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.Attachable;
+import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.RouteUnitAware;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.SQLToken;
+import org.apache.shardingsphere.infra.route.context.RouteUnit;
import java.util.Collection;
+import java.util.Map;
/**
* Projections token.
*/
-public final class ProjectionsToken extends SQLToken implements Attachable {
+public final class ProjectionsToken extends SQLToken implements Attachable, RouteUnitAware {
- private final Collection<String> projections;
+ private final Map<RouteUnit, Collection<String>> projections;
- public ProjectionsToken(final int startIndex, final Collection<String> projections) {
+ public ProjectionsToken(final int startIndex, final Map<RouteUnit, Collection<String>> projections) {
Review comment:
The original struct is `Collection<String> projections`,it's a list of rewrited derived projection string,such as generate from `order by`, how about add some comments?
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489297004
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractUtils.java
##########
@@ -198,7 +198,12 @@ public void extractTablesFromUpdate(final UpdateStatement updateStatement) {
}
}
- private boolean needRewrite(final OwnerSegment owner) {
+ /**
Review comment:
This util have state,can not call it directly by classname.
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489904704
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ProjectionsToken.java
##########
@@ -18,26 +18,29 @@
package org.apache.shardingsphere.sharding.rewrite.token.pojo;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.Attachable;
+import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.RouteUnitAware;
import org.apache.shardingsphere.infra.rewrite.sql.token.pojo.SQLToken;
+import org.apache.shardingsphere.infra.route.context.RouteUnit;
import java.util.Collection;
+import java.util.Map;
/**
* Projections token.
*/
-public final class ProjectionsToken extends SQLToken implements Attachable {
+public final class ProjectionsToken extends SQLToken implements Attachable, RouteUnitAware {
- private final Collection<String> projections;
+ private final Map<RouteUnit, Collection<String>> projections;
- public ProjectionsToken(final int startIndex, final Collection<String> projections) {
+ public ProjectionsToken(final int startIndex, final Map<RouteUnit, Collection<String>> projections) {
Review comment:
The original struct is `Collection<String> projections`,it's a list of rewrited derived projection string,such as `order by`, how about add some comments?
----------------------------------------------------------------
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] coveralls edited a comment on pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#issuecomment-693228193
## Pull Request Test Coverage Report for [Build 14833](https://coveralls.io/builds/33499812)
* **41** of **44** **(93.18%)** changed or added relevant lines in **4** files are covered.
* **13466** unchanged lines in **9** files lost coverage.
* Overall coverage increased (+**0.02%**) to **35.069%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-rewrite%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Frewrite%2Ftoken%2Fgenerator%2Fimpl%2FProjectionsTokenGenerator.java#L79) | 37 | 40 | 92.5%
<!-- | **Total:** | **41** | **44** | **93.18%** | -->
| Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/config/YamlShadowRuleConfiguration.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fconfig%2FYamlShadowRuleConfiguration.java#L42) | 1 | 50.0% |
| [shardingsphere-governance/shardingsphere-governance-core/shardingsphere-governance-core-config/src/main/java/org/apache/shardingsphere/governance/core/config/ConfigCenter.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-governance%2Fshardingsphere-governance-core%2Fshardingsphere-governance-core-config%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fgovernance%2Fcore%2Fconfig%2FConfigCenter.java#L216) | 2 | 89.92% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-common/src/main/java/org/apache/shardingsphere/shadow/yaml/swapper/ShadowRuleConfigurationYamlSwapper.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fyaml%2Fswapper%2FShadowRuleConfigurationYamlSwapper.java#L41) | 3 | 70.0% |
| [shardingsphere-features/shardingsphere-shadow/shardingsphere-shadow-api/src/main/java/org/apache/shardingsphere/shadow/api/config/ShadowRuleConfiguration.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-features%2Fshardingsphere-shadow%2Fshardingsphere-shadow-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fshadow%2Fapi%2Fconfig%2FShadowRuleConfiguration.java#L43) | 5 | 0% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/MySQLErrPacketFactory.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2FMySQLErrPacketFactory.java#L72) | 5 | 79.17% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/execute/MySQLComStmtExecuteExecutor.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2Fcommand%2Fquery%2Fbinary%2Fexecute%2FMySQLComStmtExecuteExecutor.java#L54) | 6 | 70.97% |
| [shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/text/query/MySQLComQueryPacketExecutor.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-proxy%2Fshardingsphere-proxy-frontend%2Fshardingsphere-proxy-frontend-mysql%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fproxy%2Ffrontend%2Fmysql%2Fcommand%2Fquery%2Ftext%2Fquery%2FMySQLComQueryPacketExecutor.java#L53) | 7 | 79.41% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementBaseVisitor.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementBaseVisitor.java#L811) | 18 | 2.16% |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/target/generated-sources/antlr4/org/apache/shardingsphere/sql/parser/autogen/PostgreSQLStatementParser.java](https://coveralls.io/builds/33499812/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-postgresql%2Ftarget%2Fgenerated-sources%2Fantlr4%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fautogen%2FPostgreSQLStatementParser.java#L652) | 13419 | 18.06% |
<!-- | **Total:** | **13466** | | -->
| Totals | [](https://coveralls.io/builds/33499812) |
| :-- | --: |
| Change from base [Build 14818](https://coveralls.io/builds/33494239): | 0.02% |
| Covered Lines: | 36574 |
| Relevant Lines: | 104293 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491877550
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
maybe extractor is a better name
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489305083
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -67,4 +92,40 @@ private String getDerivedProjectionText(final Projection projection) {
}
return projection.getExpression() + " AS " + projection.getAlias().get() + " ";
}
+
+ private String getDerivedProjectionTextFromColumnOrderByItemSegment(final DerivedProjection projection, final TableExtractUtils utils, final RouteUnit routeUnit) {
+ Preconditions.checkState(projection.getAlias().isPresent());
+ Preconditions.checkState(projection.getRealProjection() instanceof ColumnOrderByItemSegment);
+ ColumnOrderByItemSegment columnOrderByItemSegment = (ColumnOrderByItemSegment) projection.getRealProjection();
+ ColumnOrderByItemSegment newColumnOrderByItem = generateNewColumnOrderByItem(columnOrderByItemSegment, routeUnit, utils);
+ return newColumnOrderByItem.getText() + " AS " + projection.getAlias().get() + " ";
+ }
+
+ private Optional<String> getActualTables(final RouteUnit routeUnit, final String logicalTableName) {
+ for (RouteMapper each : routeUnit.getTableMappers()) {
+ if (each.getLogicName().equalsIgnoreCase(logicalTableName)) {
+ return Optional.of(each.getActualName());
+ }
+ }
+ return Optional.empty();
+ }
+
+ private ColumnOrderByItemSegment generateNewColumnOrderByItem(final ColumnOrderByItemSegment old, final RouteUnit routeUnit, final TableExtractUtils utils) {
+ Optional<OwnerSegment> ownerSegment = old.getColumn().getOwner();
+ if (!ownerSegment.isPresent()) {
+ return old;
+ }
+ if (!utils.needRewrite(ownerSegment.get())) {
+ return old;
+ }
+ Optional<String> actualTableName = getActualTables(routeUnit, ownerSegment.get().getIdentifier().getValue());
+ Preconditions.checkState(actualTableName.isPresent());
+ ColumnSegment newColumnSegment = new ColumnSegment(0, 0, old.getColumn().getIdentifier());
+ IdentifierValue newOwnerIdentifier = new IdentifierValue(ownerSegment.get().getIdentifier().getQuoteCharacter().getStartDelimiter()
Review comment:
Ok, i change it to String.format.
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491919555
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
Ok
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r489227432
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ProjectionsTokenGenerator.java
##########
@@ -26,16 +32,26 @@
import org.apache.shardingsphere.sql.parser.binder.segment.select.projection.impl.DerivedProjection;
import org.apache.shardingsphere.sql.parser.binder.statement.SQLStatementContext;
import org.apache.shardingsphere.sql.parser.binder.statement.dml.SelectStatementContext;
-import org.apache.shardingsphere.infra.rewrite.sql.token.generator.OptionalSQLTokenGenerator;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.dml.column.ColumnSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.dml.order.item.ColumnOrderByItemSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.segment.generic.OwnerSegment;
+import org.apache.shardingsphere.sql.parser.sql.common.util.TableExtractUtils;
+import org.apache.shardingsphere.sql.parser.sql.common.value.identifier.IdentifierValue;
import java.util.Collection;
+import java.util.HashMap;
import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
import java.util.stream.Collectors;
/**
* Projections token generator.
*/
-public final class ProjectionsTokenGenerator implements OptionalSQLTokenGenerator<SelectStatementContext>, IgnoreForSingleRoute {
+public final class ProjectionsTokenGenerator implements OptionalSQLTokenGenerator<SelectStatementContext>, IgnoreForSingleRoute, RouteContextAware {
+
+ @Setter
+ private RouteContext routeContext;
Review comment:
Any reason to add setter routeContext here?
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491658545
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
I think it is just a util class with status, what folder do you think is more appropriate?
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476#discussion_r491877550
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/util/TableExtractor.java
##########
@@ -46,7 +46,7 @@
import java.util.LinkedList;
import java.util.Optional;
-public final class TableExtractUtils {
+public final class TableExtractor {
Review comment:
maybe extractor is a better name
----------------------------------------------------------------
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 #7476: Rewrite derived projection from orderby
Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #7476:
URL: https://github.com/apache/shardingsphere/pull/7476
----------------------------------------------------------------
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