morningman opened a new pull request, #69350:
URL: https://github.com/apache/doris/pull/69350
### What problem does this PR solve?
Issue Number: None
Related PR: #69194
Problem Summary:
**In short.** The Variant write check added in #69194 also rejects writes
that lose nothing: a scalar field of a composite target that is computed from a
Variant, and a Variant that is only compared or only picks an element. This PR
traces only the parts of a value that reach a Variant leaf of the target
column, and skips the arguments that only compare or select. A lossy cast on a
value that is written into a Variant leaf is still rejected. This is the
follow-up promised in two review threads of #69194.
**Background.** Common-type analysis runs before the sink is bound. When a
Variant meets a number in a UNION, IF or CASE, the Variant side is implicitly
cast to DECIMAL, and an object or array Variant becomes SQL NULL; the sink then
encodes that NULL or scalar back into the Variant column.
`VariantWritePlanValidator` (#69194), which every connector's INSERT /
OVERWRITE / CTAS / row-level DML goes through, follows the lineage of each
target column that contains a Variant and rejects an implicit
Variant-to-non-Variant cast on the way. It already skips the positions that
only choose rows or values: IF and CASE conditions, window partition and order
keys, the ORDER BY of an aggregate function, and the later inputs of EXCEPT and
INTERSECT.
**The problem, and what it cost.** Two shapes of correct writes fail with
"VARIANT write cannot safely convert input column ...":
1. As soon as a target column contains one Variant leaf, the whole column is
traced, so an implicit cast in a field that is written as a BOOLEAN is treated
as a Variant loss.
2. Every argument of every expression is treated as a written value,
including arguments that are only compared or only select an element.
| Write (`v` is a VARIANT) | Target | Before | After |
|---|---|---|---|
| `named_struct('matched', COALESCE(v['n'], 0) > 1, 'payload', v)` |
`STRUCT<matched BOOLEAN, payload VARIANT>` | rejected | accepted |
| the same struct built from subquery columns, from a struct column's
fields, through `struct()`, a cast between structs, an IF or a UNION | same |
rejected | accepted |
| `map(IF(TRUE, v['k'], 1), v)` | `MAP<STRING, VARIANT>` | rejected |
accepted |
| `v['n'] > 0`, `v['n'] IN (1, 2)`, `v['s'] LIKE 'a%'`, `NOT v['flag']` |
`VARIANT` | rejected | accepted |
| `NULLIF(1, IF(TRUE, v, 1))`, `element_at(ARRAY(v), IF(TRUE, v['n'], 1))` |
`VARIANT` | rejected | accepted |
| `named_struct('matched', TRUE, 'payload', IF(TRUE, v, 1))` |
`STRUCT<matched BOOLEAN, payload VARIANT>` | rejected | rejected |
| `IF(TRUE, v, 1) > 0`, `NULLIF(IF(TRUE, v, 1), 1)`,
`element_at(ARRAY(IF(TRUE, v, 1)), 1)` | `VARIANT` | rejected | rejected |
The only workaround was an explicit CAST around the sibling field or the
comparison.
**How this PR fixes it.**
1. The validator traces a demand per ExprId instead of a set of ExprIds. A
demand says which parts of a value reach a Variant leaf: none, all of it, or
the parts of each field of a STRUCT, of the element of an ARRAY, or of the key
and the value of a MAP. The demand of a target column comes from its type and
is aligned position by position to the source type, as the sink's struct cast
is; when the shapes differ, all of the value is traced, as before.
2. A closed set of structural expressions passes the demand on:
`named_struct` / `struct` (field i to its argument), `array` and `map`
constructors, `element_at` (a struct field chosen by its constant name or
1-based position, an array element, a map value), casts (part by part between
values of the same shape), the results of IF / CASE / NULLIF / NVL
(`ExpressionUtils.getCaseWhenLikeBranchResults`, the model
`JoinExtractOrFromCaseWhen` already relies on), aliases, window functions, set
operation inputs by ordinal, recursive CTEs, CTE outputs and scalar subqueries.
Any other expression traces all of every argument, as before, so an expression
the validator does not model can only reject more. A cast is lossy only if it
loses a Variant leaf in a demanded part.
3. Three kinds of argument are not traced, because they never become the
written value: the second argument of NULLIF, the index or key of `element_at`,
and the implicit cast that a comparison, IN, LIKE / REGEXP, AND / OR or NOT
applies directly to an operand. Only that one cast is skipped; the operand
under it is still traced, so `IF(TRUE, v, 1) > 0` is still rejected. Functions
whose arguments may be returned are not listed one by one: `ELT(v['n'], 'a',
'b')`, `SUBSTRING('abc', v['n'])` and `MAX_BY(1, IF(TRUE, v, 1))` are still
rejected and need an explicit CAST.
4. A CTE output is traced once per demand rather than once: it is traced
again only when a consumer requests a part not traced yet, at most once per
nested field of its type, so the work stays proportional to the plan (the two
existing exponential-plan tests still run in milliseconds). The recursive CTE
fixpoint works the same way per output ordinal.
5. The error message now ends with "use an explicit CAST if the conversion
is intended".
branch-4.1 and branch-4.2 scan the whole write expression for such casts,
including IF and CASE conditions, so they reject more than master; this PR does
not change them.
**Results.**
- `VariantWritePlanValidatorTest`: six new tests (struct targets in seven
shapes, crossed struct fields by name and by position, ARRAY<STRUCT> and MAP
targets, values of another shape, compared and selecting arguments, a CTE
output traced again for more of it). Each accepted statement was checked to be
rejected before this change, and each rejected one to stay rejected. 19
mutations of the new rules (dropping each structural rule, swapping map keys
and values, an off-by-one struct position, tracing the skipped arguments,
reusing a CTE trace for any part, dropping the demand on a shape mismatch, and
others) each fail at least one test. The 21 existing tests pass unchanged.
- `test_paimon_write_variant_errors`: a Paimon table with `STRUCT<matched:
BOOLEAN, payload: VARIANT>` and a top-level VARIANT column. The scalar field
computed with `COALESCE(payload['n'], 0) > 1`, in place and from a subquery,
and the comparisons `payload['n'] > 1` and `payload['kind'] LIKE 'object%'` are
written and read back (Spark reads the same values); a loss in
`flagged.payload` or under a comparison operand is still rejected. Before this
change the first of these writes fails with the error above.
- The other Variant write suites, all six `test_paimon_write_variant*` and
`test_iceberg_variant_read`, pass locally with this change.
### Classes and call graph
```
BindSink / BindExpression
-> VariantWritePlanValidator.validateNoLossyCoercion(targets, plan[,
sourceOrdinals][, cteContext])
Demand.of(target type).alignTo(source type)
-> traceOutputLineage(plan, ExprId -> Demand)
set operation / recursive CTE / generate / CTE consumer:
per-ordinal demands
other plans -> validateExpression(named expression, demand)
-> valueNodes(expression, demand) structural
dispatch, skipped arguments
-> validateLossyCast(nodes)
losesVariantLeaf(source, target, demand)
-> slots and scalar subqueries -> demands on the child plans
```
### Release note
Writes into an external table column that contains a VARIANT no longer fail
when a Variant is implicitly cast only to compute a non-Variant field of the
target (for example the BOOLEAN field of a STRUCT), only to be compared
(comparison, IN, LIKE, REGEXP, AND, OR, NOT), or only to choose a value (the
second argument of NULLIF, the index or key of element_at). The error for a
lossy cast now suggests an explicit CAST.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- Behavior changed:
- [ ] No.
- [x] Yes. Writes described in the release note are accepted instead of
rejected; the error message gains a hint.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
--
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]
