ClaireLytt commented on PR #39131:
URL: https://github.com/apache/shardingsphere/pull/39131#issuecomment-5115851883
@terrymanu
Thank you for the detailed review. I now understand the core issues:
State ambiguity: The current fix uses column-count/label mismatch as a proxy
to detect the "unbound shorthand" state, but this conflates two opposite
scenarios — HintManager skipping bind (missing user-visible columns) vs. normal
rewrite adding internal derived columns (e.g. ORDER_BY_DERIVED_0). The former
should trust physical metadata, while the latter must hide internal columns.
Contract split: The PR only changes columnLabelAndIndexMap (used by
ResultSet#getObject(String)) but does not update
ShardingSphereResultSetMetaData, which still reports column count and labels
from expandProjections. After the merge, the two JDBC-facing APIs describe
different result shapes.
Root cause: When HintManager.setDataSourceName(...) is present,
SQLBindEngine.isNeedBind() returns false, so ShorthandProjectionSegmentBinder
never expands T.*. The ProjectionsContext then only contains the explicit
projection (status_new), while the physical result set has all columns. This is
the actual state that needs to be explicitly represented — not inferred from
mismatch.
I'll pause the current implementation direction. Before re-implementing, I'd
like to align on the design:
**Proposed approach:**
- **Step 1**: Add an explicit flag (e.g. `boundWithCompleteProjections`) on
`SelectStatementContext` or `SelectStatementBaseContext`, set by
`SQLBindEngine` — `true` when bind was executed, `false` when skipped due to
data-source hint. **Purpose**: make the bind state explicitly queryable
downstream, instead of inferring it from column-count or label mismatch.
- **Step 2**: Introduce a unified "visible result columns" resolution point
that checks this flag:
- `true` → use `expandProjections` (filters out `DerivedProjection` as
today)
- `false` → use physical `ResultSetMetaData`, filtering out any
`DerivedColumn.isDerivedColumnName(...)` labels
**Purpose**: produce one authoritative "which columns the user should see"
answer, regardless of which path was taken, so that HintManager's unbound
shorthand and normal rewrite's internal derived columns are handled with
opposite — and correct — logic.
- **Step 3**: Have `ResultSet#getObject(String)`, `ResultSetMetaData`, and
Proxy `QueryHeaderBuilderEngine` all consume from this single resolution point.
**Purpose**: ensure all JDBC-facing contracts share the same result shape,
eliminating the current split where `getObject` sees a merged map while
`getMetaData` still reports from `expandProjections`.
Hopefully, the ideas in my proposed approach address the four questions
raised in your latest review comments. Does this direction make sense, or do
you have a different ownership boundary in mind? Happy to discuss before coding.
--
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]
