924060929 commented on PR #62905:
URL: https://github.com/apache/doris/pull/62905#issuecomment-4408084315
## Simpler fix: validate in `castUnbound` instead of adding rewrite rules
The bug is real — mismatched `variant<config_A>` → `variant<config_B>` casts
slip through FE and crash BE. But the fix can be much simpler and avoid adding
overhead to the INSERT VALUES fast path.
### Root cause
`TypeCoercionUtils.castUnbound()` has two branches:
- **Literal**: calls `castIfNotSameType()` → calls `checkCanCastTo()` ✅
- **Non-Literal**: calls `unSafeCast()` → skips validation entirely ❌
For `cast('json' as variant<config_A>)`, the parser produces a `Cast` node
whose `getDataType()` returns the fully-resolved `VariantType` (with all
properties from SQL text or session variables) at **parse time** — no
analysis/binding needed. So we can validate it right there.
### Proposed fix (one line)
```java
public static Expression castUnbound(Expression expression, DataType
targetType) {
if (expression instanceof Literal) {
return TypeCoercionUtils.castIfNotSameType(expression, targetType);
} else {
+ try {
+ checkCanCastTo(expression.getDataType(), targetType);
+ } catch (UnboundException e) {
+ // Unbound expressions (e.g. function calls) — validated after
analysis
+ }
return TypeCoercionUtils.unSafeCast(expression, targetType);
}
}
```
### Why this works
`Cast.getDataType()` returns `targetType` — it's declarative (set at
construction), not derived from binding. So at `castUnbound` time:
- `cast('json' as variant<A>)` → `expression.getDataType()` = `variant<A>` ✅
known
- `bitmap_hash('x')` → `UnboundFunction.getDataType()` throws
`UnboundException` → caught and skipped
I verified this by building a test cluster from master, adding
`checkCanCastTo` to `castUnbound`, and running the PR's test cases:
| Test case | Result |
|---|---|
| `cast(json as variant<mismatch>)` single row | **Rejected at FE** ✅ |
| `cast(json as variant<mismatch>)` multi-row VALUES | **Rejected at FE** ✅ |
| `cast(json as variant<mismatch>)` in txn (BEGIN) | **Rejected at FE** ✅ |
| Normal `insert into t values ('json')` | Works ✅ |
| `cast(json as variant)` matching config | Works ✅ |
| Multi-row normal VALUES | Works ✅ |
### Why the current PR approach is heavier than needed
The current PR:
1. Adds `CheckCast.INSTANCE` to `INSERT_JOBS` and `BATCH_INSERT_JOBS`
rewrite pipelines
2. Adds `LogicalSetOperationRewrite` to `RewriteInsertIntoExpressions` (to
visit `LogicalUnion.constantExprsList`)
3. Changes `LogicalSetOperationRewrite` visibility from `private` to `public`
This adds an expression tree traversal to the INSERT VALUES fast path —
which exists specifically to skip full rewrite/optimize. The
`LogicalSetOperationRewrite` visits every expression in every row of a
multi-row INSERT, costing O(N×C) for N rows × C columns.
The `castUnbound` fix catches the same bug at the source, before the
expression tree is even constructed, at O(1) per value.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
