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:
[email protected]
