terrymanu commented on PR #39131: URL: https://github.com/apache/shardingsphere/pull/39131#issuecomment-5326759018
The revised design direction matches the expected ownership and mapping boundary, and the design discussion is resolved at the proposal level. To be explicit about where this leaves the PR: the previous review result still stands. Nothing in the current head is approved by this reply — it only unblocks the re-implementation, and the PR remains not mergeable until the new implementation lands with the agreed tests and a green CI run. **Current head state** - The single commit of 2026-07-15 now conflicts with master (`mergeable_state: dirty`). - 24 of the 88 checks on that head are red, all of them in the `E2E - SQL (Smoke)` matrix across every adapter, mode, database, and scenario combination. - Whether those failures come from this PR's own delta or from the stale base is not worth establishing, because the current implementation is superseded by the redesign below. Please do not spend time fixing the current head or its CI; rebase onto the latest master and build the new implementation there, so the conflicts and the red smoke matrix are discarded together. **Design pillars confirmed against the current code** 1. `ProjectionsContext` as the owner of the unexpanded-shorthand state is correct and requires no new data model. `ProjectionEngine` builds `ShorthandProjection` from `ShorthandProjectionSegment.getActualProjectionSegments()`, which only `ShorthandProjectionSegmentBinder` populates during binding (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/projection/type/ShorthandProjectionSegmentBinder.java#L83-L89). When `SQLBindEngine.isNeedBind()` returns false, that collection stays empty, and `createExpandProjections()` silently drops the shorthand today (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/ProjectionsContext.java#L84-L94). A query method such as `containsUnexpandedShorthandProjection()` can therefore expose the state without size or label inference. 2. For internal columns, consume the existing explicit representation instead of introducing a parallel one. Internally generated projections are already tracked: `ORDER_BY_DERIVED_n` / `GROUP_BY_DERIVED_n` are created as `DerivedProjection` by `ProjectionsContextEngine.getDerivedOrderColumns()` (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionsContextEngine.java#L86-L96), AVG-derived `COUNT`/`SUM` projections live on `AggregationProjection.getDerivedAggregationProjections()`, and the rewriter injects exactly those objects (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/rewrite/token/generator/impl/ShardingProjectionsTokenGenerator.java). The current branch on `containsDerivedProjections()` in `ShardingSphereResultSetUtils.createColumnLabelAndIndexMap()` (jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/resultset/ShardingSphereResultSetUtils.java#L44-L53) is the maintain ed precedent for consuming that explicit knowledge. 3. The shared visible result-shape mapping carrying `visibleColumnIndex`, `physicalColumnIndex`, `columnLabel`, and `columnName`, consumed by `ResultSet#getObject(String)`, `ResultSetMetaData`, and the Proxy `QueryHeaderBuilderEngine` (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/response/header/query/QueryHeaderBuilderEngine.java#L98-L104 still resolves from `expandProjections`), closes the split result-shape contract. **Two clarifications to fold into the design** - Treat relabeling and hiding as two explicit resolver outcomes. `AGGREGATION_DISTINCT_DERIVED_n` (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/projection/engine/ProjectionEngine.java#L123-L124) is not a hidden column: for `SELECT COUNT(DISTINCT user_id) FROM t_order` the column is user-visible and only its physical label is internal. The mapping must express "physical label → user-visible label (the original expression)", which is what `ProjectionsContext.createColumnLabelAndIndexMap()` already keys by expression today (ProjectionsContext.java#L182). - Add a sixth test case where the two states coexist: a data-source hint with an unexpanded shorthand plus an order-by item absent from the projections, for example `SELECT T.*, T.status status_new FROM t_order T ORDER BY T.user_id`. The skipped bind does not prevent `ProjectionsContextEngine` from appending derived order-by columns, because an unexpanded shorthand cannot satisfy `containsProjection()`, so the context can hold an unexpanded shorthand and internally generated projections at the same time. The resolver must then trust physical metadata for the shorthand columns and still hide the internally generated column in one pass. **Minimum next actions** 1. Rebase onto the latest master; do not fix the current head. 2. Implement per the three pillars and two clarifications above. 3. Rebuild the test structure rather than extending the current `GeneralDQLE2EIT` provider, and cover the six scenarios, including consistent labels, counts, and indexes across `ResultSet#getObject(String)`, `ResultSetMetaData`, and Proxy query headers. 4. Restore the `RELEASE-NOTES.md` entry. 5. A green CI run on the new head is required before the next review round. -- 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]
