github-actions[bot] commented on PR #61190:
URL: https://github.com/apache/doris/pull/61190#issuecomment-4031360490
## Code Review Summary
**PR: [fix](variant) reject root variant match predicates**
This PR adds a check in `CheckMatchExpression` to reject `MATCH` predicates
on root VARIANT columns (those without a sub-column path), which would silently
fall through to a full scan without index acceleration. The fix is clean,
well-scoped, and well-tested.
### Critical Checkpoint Conclusions
- **Goal & Correctness**: The goal is to prevent users from issuing
`response MATCH 'xxx'` on a root VARIANT slot (which has no inverted index).
The code accomplishes this by checking
`slotReference.getDataType().isVariantType() && !slotReference.hasSubColPath()`
after unwinding any Cast chain. This is correct —
`ExpressionAnalyzer.visitMatch()` wraps VARIANT left operands in a `Cast`, so
the Cast-chain unwinding is necessary and handled properly.
- **Modification scope**: Minimal and focused. Only 12 lines changed in the
production file, with a clean refactor of `isSlotOrCastChainOnSlot` →
`getSlotFromSlotOrCastChain` that returns the `SlotReference` (or null) instead
of a boolean, avoiding a second traversal.
- **Concurrency**: Not applicable — this is a single-threaded rewrite rule
in the Nereids optimizer.
- **Parallel code paths**: Investigated all 12 locations where Match
expressions are validated/checked. The new variant root check is correctly
placed in `CheckMatchExpression` which runs in the "final rewrite and check"
stage. `CheckAfterRewrite.checkMatchIsUsedCorrectly()` handles placement
validation (Match only in filter above OlapScan/DeferMaterializeOlapScan). The
`CheckMatchExpression` pattern matches only `logicalFilter(logicalOlapScan())`
and not `logicalDeferMaterializeOlapScan()`, but this is benign — the
DeferMaterialize transformation does not modify filter expressions, so any
malformed Match would have already been caught before the transformation.
- **Incompatible changes**: None. This is a strictly additive check that
rejects previously-silently-broken queries.
- **Test coverage**:
- Unit tests (`CheckMatchExpressionTest`): Covers root VARIANT slot
rejection, Cast-wrapped root VARIANT rejection, and VARIANT sub-column
allowance. Uses reflection to invoke the private `checkChildren` method
directly — reasonable for a focused unit test.
- Regression test (`test_disable_root_variant_match`): Creates a VARIANT
column with MATCH_NAME inverted index, verifies root MATCH is rejected with
expected error message, and confirms sub-column MATCH returns correct results.
Follows test conventions (DROP before use, hardcoded table name,
`test{exception}` pattern for error case, ORDER BY for result ordering).
- **Configuration / observability**: No new configuration items needed. The
error message is clear and actionable, suggesting the correct `column['field']
MATCH 'xxx'` syntax.
- **Performance**: No performance impact — this is a compile-time check in
the optimizer rewrite phase.
**Verdict: No issues found.** The change is correct, minimal, and
well-tested.
--
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]
