rishi-rana commented on PR #39866: URL: https://github.com/apache/shardingsphere/pull/39866#issuecomment-5737749863
One observation, offered as a suggestion rather than a required change. `ShardingProjectionsTokenGenerator` already performs the same owner rewrite that the new `getColumnLabel(ColumnOrderByItemSegment, ...)` introduces: for a given route unit it takes a `ColumnOrderByItemSegment`, resolves the owner to the actual table, and wraps it in the owner's original quote character ([generateNewColumnOrderByItem, lines 120-132](https://github.com/apache/shardingsphere/blob/101c3d56ccd7ee3c4a390a8f37550bed58a4751c/features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L120-L132)). That generator emits the derived projection (`t_account_0.status AS ORDER_BY_DERIVED_0`) and this one emits the matching `ORDER BY`, so both resolve the same owner in the same statement, now through two independent implementations that do it differently: 1. **Table lookup.** `getActualTableName` scans `routeUnit.getTableMappers()` only ([line 134](https://github.com/apache/shardingsphere/blob/101c3d56ccd7ee3c4a390a8f37550bed58a4751c/features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L134)), while the new path uses `ShardingTokenUtils.getLogicAndActualTableMap()`, which also merges `getLogicAndActualTablesFromBindingTable(...)` ([ShardingTokenUtils line 50](https://github.com/apache/shardingsphere/blob/101c3d56ccd7ee3c4a390a8f37550bed58a4751c/features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/pojo/ShardingTokenUtils.java#L50)). A binding-table owner therefore resolves in one path but not the other. 2. **Unmapped owner.** The new path keeps the original text when the owner is not in the map; `getActualTableName` throws `IllegalStateException` ([line 140](https://github.com/apache/shardingsphere/blob/101c3d56ccd7ee3c4a390a8f37550bed58a4751c/features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java#L140)). The behaviour in this PR looks like the better of the two, since binding-table resolution is the more complete rule. The part worth raising is that the more correct version now lives in only one of the two places, so the two can drift apart as either is maintained. Would it be worth extracting the owner rewrite into `ShardingTokenUtils`, next to `getLogicAndActualTableMap`, so both generators share it? That would keep the derived projection and the generated `ORDER BY` resolving owners by the same rule. I have not constructed a query that actually diverges between the two paths, so this is a maintainability observation rather than a reported defect, and it may well be better as a follow-up since the older behaviour predates this change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
