924060929 commented on PR #65561:
URL: https://github.com/apache/doris/pull/65561#issuecomment-5100120287
I went through the FE part in detail. The overall approach looks right to
me: replacing the process-wide BE switch with a session-scoped, execution-only
type marker (`computeV2`), isolating V1/V2 in type coercion, treating V1→V2 as
non-injective in `isInjectiveCastTo` (canonical normalization can collapse
distinct values — nice catch), and the `NeedSessionVarGuard` +
`affectQueryResultInPlan` wiring for views / sql cache is complete.
However, several FE changes are **not gated by `enable_variant_v2`**, which
conflicts with the PR's stated contract ("enabling it in one session does not
change the physical Variant type used by ... other sessions"; disabled by
default). Ordered by severity:
### 1. Legacy Variant is now accepted as GROUP BY / DISTINCT keys, but BE
cannot execute it — should be fixed before merge
`CheckAfterRewrite`'s group-key check changed from `isObjectOrVariantType()`
to `isObjectType()`, which un-gates **legacy** Variant regardless of the
session variable. `VariantEqualityContextTest.testAllowedCanonicalHashContexts`
pins `GROUP BY v` / `SELECT DISTINCT v` on plain legacy `VARIANT` table columns
as accepted with the flag **off**.
On the BE side, `get_hash_key_type()` now routes every Variant key to
`HashKeyType::serialized`, but legacy `ColumnVariant` implements none of the
serialized-key entry points: `serialize_value_into_arena` / `get_data_at` throw
NOT_IMPLEMENTED, and there is no `serialize_vec` / `serialize_impl` override so
the `IColumn` defaults throw as well. This PR doesn't change legacy
`ColumnVariant`.
So `SELECT v, COUNT(*) FROM legacy_table GROUP BY v` degrades from a clean
FE AnalysisException into a BE runtime `[NOT_IMPLEMENTED_ERROR]
serialize_value_into_arena ColumnVariant` error. The negative case removed from
`load.groovy` (`group by v['a']` expecting the FE error) has no replacement
positive e2e case — I don't think one can pass. Notably
`create_aggregate_function_uniq` did get a clean legacy-rejection guard in this
PR, but the hash-agg / set-operator serialized-key path has no such guard.
Suggested fix — keep rejecting non-V2 variants, which preserves the full V2
capability and leaves legacy behavior exactly as before:
```java
DataType type = groupBy.getDataType();
if (type.isObjectType() || type.isVarBinaryType()
|| (type.isVariantType() && !((VariantType) type).isComputeV2())) {
throw new AnalysisException(Type.OnlyMetricTypeErrorMsg);
}
```
### 2. Comparison-predicate rejection also applies to legacy Variant — needs
an explicit decision + release note
`TypeCoercionUtils.processComparisonPredicate` now throws for any comparison
involving a Variant unless the Variant side is *directly* an `ElementAt` and
the other side is not a Variant. This breaks previously-working legacy SQL
regardless of the session variable; the PR's own test edits show the impact:
- bare-column comparison: `delete from t where v = 'xxx'` was removed from
`delete_update.groovy` (implicit variant→string coercion used to make this
work, DELETE predicates included);
- sub-path through an alias/subquery boundary: `SELECT dt['e'] AS c2 FROM
... WHERE c2 > 1` now fails because the outer slot is no longer an `ElementAt`
— `test_sub_path_pruning.groovy` had to be rewritten to `cast(c2 as int) > 1`.
Projecting a subpath in a subquery/view and filtering outside is a very common
shape in real workloads.
Also note the blast radius includes **existing views / MTMVs**: definitions
containing such predicates fail to re-analyze after upgrade (and MTMV refreshes
start failing), since the rejection is not session-gated and session-var replay
doesn't help here.
If the intent is to also clean up the fuzzy V1 implicit-comparison
semantics, that's a fair call — but the release note should list these breaks
explicitly (bare-column comparisons incl. DELETE predicates, alias-boundary
sub-path comparisons). Otherwise, consider throwing only when a V2 type is
involved and keeping the legacy coercion path for V1.
### 3. variant→JSON implicit coercion removed globally — same treatment as #2
`implicitCast(variant, json) → empty` plus the new `JsonType`-signature
guard in `ExplicitlyCastableSignature` block both the implicit and
explicitly-castable resolution rounds, un-gated. The PR's own edits to
long-standing passing suites show existing SQL breaks: `json_extract(v,
'$.repo')` → `v['repo']`, `json_extract(v['k5'], ...)` →
`json_extract(cast(v['k5'] as JSONB), ...)`, `json_object_flatten(v)` →
`json_object_flatten(cast(v AS JSONB))`. Same view/MTMV re-analysis concern as
#2, and the user-facing failure is a generic "can not find function" rather
than a targeted message.
### 4. `CAST(legacy_column AS VARIANT)` with the flag on fails at BE runtime
— minor
With `enable_variant_v2 = true`, `ExpressionAnalyzer.visitCast` marks every
explicit Variant cast target as V2, and `CheckCast.check` accepts any
variant→variant pair when the flag is on. So `CAST(v AS VARIANT)` on a legacy
column — previously an identity cast — now reaches BE and fails with "Cast
between legacy Variant and compute-only Variant V2 is not supported". Since BE
explicitly doesn't support it, FE could reject V1-source→V2-target in
`CheckCast` with the same message and fail at analysis time. This scenario
currently has no e2e coverage (`variant_compute_v2.groovy` only builds V2
values from expressions).
### 5. Nit: stale description
The PR description says `parse_to_variant_error_to_null`, but the registered
function is `try_parse_to_variant` (renamed in the last commit).
--
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]
