ClaireLytt commented on PR #39131:
URL: https://github.com/apache/shardingsphere/pull/39131#issuecomment-5276587146
@terrymanu Thank you for the clarification. I will revise the design before
implementation.
## Root Cause
The root cause is not simply a column-count or label mismatch. The real
state that matters is whether a shorthand projection remains unexpanded.
For the HintManager case:
```sql
SELECT T.*, T.status status_new FROM t_order T WHERE T.order_id = 1000
```
`T.*` may remain unexpanded, so `ProjectionsContext` only contains the
explicit projection (`status_new`), while the physical result set contains all
user-visible columns.
The current fix also creates a split contract: `ResultSet#getObject(String)`
uses the merged `columnLabelAndIndexMap`, while `ResultSetMetaData` still
reports from `expandProjections`.
## Proposed Changes
### 1. Problem: bind state does not equal projection completeness
A data-source hint may skip SQL binding, but that does not always mean the
projection list is incomplete.
For example, this hinted query has only explicit projections:
```sql
SELECT T.order_id, T.status status_new FROM t_order T WHERE T.order_id = 1000
```
Even if binding is skipped, the user-visible projection list is already
complete:
```text
order_id, status_new
```
The real problematic case is a hinted query with shorthand projection:
```sql
SELECT T.*, T.status status_new FROM t_order T WHERE T.order_id = 1000
```
Here `T.*` needs binding to expand into actual columns. If it remains
unexpanded, `ProjectionsContext` may only contain the explicit projection
`status_new`, while the physical result set contains `order_id`, `user_id`,
`status`, and `status_new`.
**Solution**: Do not derive the state from `SQLBindEngine.isNeedBind()`.
Move the state ownership to `ProjectionsContext`, and let it explicitly expose
whether it contains an unexpanded shorthand projection.
### 2. Problem: internal columns cannot be identified only by name prefix
Normal SQL rewrite may add internal columns for merge, for example:
```sql
SELECT t_account.amount FROM t_account ORDER BY t_account.account_id ASC
```
may be rewritten to:
```sql
SELECT t_account_0.amount, t_account_0.account_id AS ORDER_BY_DERIVED_0
FROM t_account_0
ORDER BY t_account_0.account_id ASC
```
`ORDER_BY_DERIVED_0` is an internal column and must not be exposed to users.
However, filtering by name alone is unsafe:
```java
DerivedColumn.isDerivedColumnName("ORDER_BY_DERIVED_0")
```
because a real physical table could also have a user-defined column with the
same name.
**Solution**: I will not hide physical metadata columns only because their
labels match reserved derived-column prefixes such as `ORDER_BY_DERIVED_`.
Instead, I will make the resolver use explicit internal-projection
information from the current statement. For example, when ShardingSphere
generates an internal projection such as `ORDER_BY_DERIVED_0`, that projection
should be represented as an internally generated projection in the
statement/projection context. The resolver will build an internal-column set
from those generated projections first, and then scan the physical
`ResultSetMetaData`.
A physical metadata column will be hidden only when it matches an internally
generated projection that ShardingSphere created for this statement. If a real
physical table happens to have a user-defined column named
`ORDER_BY_DERIVED_0`, but there is no corresponding internally generated
projection for the current statement, I will keep it visible.
This means the filtering rule becomes:
```text
hide the column only if:
it is present in physical metadata
and it corresponds to an explicitly generated internal projection of this
statement
```
### 3. Problem: hiding physical columns changes index mapping
If the physical result set is:
```text
physical metadata:
1 amount
2 ORDER_BY_DERIVED_0
3 status
```
and `ORDER_BY_DERIVED_0` is an internal column, the user-visible result
should be:
```text
visible columns:
1 amount -> physical index 1
2 status -> physical index 3
```
So the visible column index is not always the same as the physical
result-set index.
**Solution**: Introduce a shared runtime visible result-shape mapping. Each
visible column item should contain:
```text
visibleColumnIndex
physicalColumnIndex
columnLabel
columnName
```
`ResultSet#getObject(String)`, `ResultSetMetaData`, and Proxy query headers
should all consume this same mapping, so labels, counts, and indexes are
resolved from one authoritative result shape.
## Test Cases
I will cover these cases before implementation:
1. Data-source hint with unexpanded shorthand projection:
```sql
SELECT T.*, T.status status_new FROM t_order T WHERE T.order_id = 1000
```
2. Data-source hint with only explicit projections:
```sql
SELECT T.order_id, T.status status_new FROM t_order T WHERE T.order_id = 1000
```
3. Normal rewrite with internal derived columns:
```sql
SELECT t_account.amount FROM t_account ORDER BY t_account.account_id ASC
```
where the rewritten SQL may include `ORDER_BY_DERIVED_0`, which must not be
exposed.
```sql
SELECT t_account.ORDER_BY_DERIVED_0 FROM t_account
```
A real user column named ORDER_BY_DERIVED_0 must remain visible.
```sql
SELECT t_account.amount AS ORDER_BY_DERIVED_0 FROM t_account
ORDER BY t_account.account_id ASC
```
A user-defined alias named ORDER_BY_DERIVED_0 must remain visible, while the
internally generated merge column must be hidden.
4. Equal column counts but different labels.
```sql
SELECT COUNT(DISTINCT user_id) FROM t_order
```
ProjectionEngine assigns the internal alias AGGREGATION_DISTINCT_DERIVED_0
when the user does not provide one, so the physical result set reports one
column labeled AGGREGATION_DISTINCT_DERIVED_0. Meanwhile
ProjectionsContext.createColumnLabelAndIndexMap keys the same column by its
expression COUNT(DISTINCT user_id), because the label matches a reserved
derived-column prefix.
Both sides have exactly one column, but the labels differ. A resolver must
not treat this as an unexpanded-shorthand case, and must not expose
AGGREGATION_DISTINCT_DERIVED_0 to users.
5. Consistent labels, counts, and indexes across
`ResultSet#getObject(String)`, `ResultSetMetaData`, and Proxy query headers.
For each case above, I will verify that `ResultSet#getObject(String)`,
`ResultSetMetaData`, and Proxy query headers consume the same visible
result-shape mapping.
Specifically:
- `ResultSet#getObject(String)` should resolve only user-visible labels, and
each label should map to the correct physical result-set index.
- `ResultSetMetaData#getColumnCount()` should report the visible column
count from the same mapping.
- `ResultSetMetaData#getColumnLabel(int)` should report visible labels from
the same mapping and in the same order.
- Proxy query headers should expose the same visible labels and order as
`ResultSetMetaData`.
- Internal generated columns should not be visible through any of the three
consumers.
Does this revised direction match your expected ownership and mapping
boundary?
--
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]
