MDEV-39525 [wip] Add supertype checks to vcol index substitution in W…

[mariadb-YuchenPei]

…HERE

• check if both the vcol expr and vcol field are supertypes to the RHS
• check if vcol field is a supertype to vcol expr

If either check returns true, then the substitution is safe.

Skip the checks for IS NULL / IS NOT NULL.

TODO: Does not yet work for the JSON_EXTRACT/JSON_VALUE vcol examples in vcol_sargable because capacity_limit_is_in_characters returns different values for varchar and blob/text

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mariadb-YuchenPei]

claude:

---

Code Review: MDEV-39525 [wip] — Supertype checks for vcol index substitution in WHERE

Commit 94021ab — Add supertype checks to vcol index substitution in WHERE
6 files, +128 / −47 · touches sql/opt_vcol_substitution.cc, sql/field.{h,cc}, sql/item.h, and the vcol_sargable test.

Overview

Virtual-column substitution lets the optimizer rewrite WHERE a+1 <= 2 into WHERE vcol1 <= 2 so an index on the generated column vcol1 AS (a+1) can be used. That rewrite is only sound if the stored/indexed column value faithfully represents the expression value for the comparison being performed.

This commit replaces the old WHERE-path gate (type_handler_for_comparison() equality + nullability + collation) with a type-capacity ("supertype") model, unifying it with the ORDER/GROUP BY path:

• Baseline (all comparisons): substitute only if vcol_field is a supertype of vcol_expr — i.e. the column can represent every value the expression can produce (is_supertype is essentially a max_length/decimals/sign/charset capacity check, per sql/sql_type.h:5111, :5279).
• Relaxation for =, BETWEEN, IN: if both vcol_field and vcol_expr are supertypes of the RHS constant(s) (vcol_type_both_bounded), the supertype-of-expr check is skipped — the constant fits losslessly on both sides, so equality/containment is preserved even when the expression's domain is wider than the column.
• IS NULL / IS NOT NULL skip the supertype check entirely.

The reasoning is sound: it correctly stops substituting when the generated column could clamp/truncate an out-of-range expression value (e.g. a+1 overflowing INT), which the old comparison-handler-equality check missed.

Correctness

• The .result "regressions" are intentional correctness fixes, not bugs. a+1 <= 2 now does a full scan because is_supertype sees the INT column's max_length as smaller than the a+1 expression's, so near INT overflow the indexed value could diverge from the true expression value. Good. However, confirm this trade-off is acceptable — this removes a real optimization for a very common pattern (int_col_expr <op> const) where most users never hit overflow. The commit message's TODO only flags the JSON/varchar cases; the plain-int range losses aren't called out, so please confirm they're understood/intended.

• DBUG_ASSERT(0) now guards the WHERE path (potential new crash). In the old code the nullability branch with DBUG_ASSERT(0) lived only in the ORDER/GROUP-BY else. WHERE mismatches were handled non-fatally as "type mismatch". After unification (opt_vcol_substitution.cc:461), a WHERE substitution with vcol_expr->maybe_null() && !vcol_field->maybe_null() would now hit DBUG_ASSERT(0) and abort debug builds. This is safe only if an indexed virtual column can never be NOT NULL. Please verify that invariant holds on the WHERE path before relying on the assert here.

• <=> (NULL-safe equal) does not get the relaxation. other is set only for functype() == EQ_FUNC (:544, :550), so EQUAL_FUNC always takes the strict supertype-of-expr path even though vcol_subst_analyzer admits BITMAP_EQUAL. Conservative and safe, but likely an unintended asymmetry — worth a comment or a functype() == EQ_FUNC || functype() == EQUAL_FUNC if <=> should behave like =.

• IN logic is correct: skip = (i == arg_count) requires every element to be both-bounded, otherwise it falls back to the strict check. Good.

Code quality & style

• Stray indentation at opt_vcol_substitution.cc:458 — fail_cause="failed supertype check"; is indented 6 spaces (leftover from the removed nested block). The single-statement { } block is also now redundant.
• Inconsistent assignment spacing: fail_cause="failed supertype check" vs fail_cause= "collation mismatch". MariaDB style is x= y.
• Duplicated call sites — the if (both_bounded) subst(...true) else subst(...false) pattern appears twice (:553-556, :573-576). Collapse to a bool:

  bool skip= other && vcol_type_both_bounded(vcol_field, *vcol_expr, other);
  subst_vcol_if_compatible(ctx, this, vcol_expr, vcol_field, skip);

• uint i= 1; in Item_func_in (:596) is declared well above its loop; scope it to the for.
• The removed /* WHERE substitution (MDEV-39525) */ marker was useful context; consider a short comment explaining the new both-bounded relaxation rationale, since it's subtle.
• The new Field::is_supertype (field.cc:11603) and Item::is_supertype (item.h:2857) wrappers are a good cleanup — they de-duplicate the verbose 5-arg call. Note they take Item* non-const; fine here, but worth keeping const-correct if callers expand.

Test coverage

• Good: the .test exercises =, <, <=, >, >=, BETWEEN, IN, IS [NOT] NULL, views, and collation variants, and the .result deltas document the new decline reasons.
• Blocking for merge (WIP): the test contains ## TODO … capacity_limit_is_in_characters comments and set collation_connection=… workarounds that must not ship. Several JSON/varchar cases currently produce "failed supertype check" where index use is actually desired — the underlying capacity_limit_is_in_characters varchar-vs-blob discrepancy needs resolving before this is complete.
• Missing: an explicit test proving the soundness motive — a row where a+1 overflows the column type and a range query that returns the correct (non-indexed) result, contrasted against a value-preserving case. That would lock in why the substitution is refused.

Performance / security

• No security concerns. Performance impact is at plan time only (a couple of extra is_supertype capacity checks); negligible. The meaningful cost is the loss of index access for the now-rejected patterns — see the correctness note above; that's the main thing to weigh.

Summary

The direction is correct and the both-bounded relaxation is a clean way to keep the common = / BETWEEN / IN const cases fast while closing the overflow-soundness hole. Before this leaves WIP: (1) resolve the capacity_limit_is_in_characters TODO so the JSON/varchar cases regain their indexes, (2) confirm the DBUG_ASSERT(0) is unreachable on the WHERE path, (3) decide whether plain-int range losses are acceptable, and (4) clean up the stray indentation, duplicated branches, and test scaffolding.

I've kept this to a review only and haven't pushed anything or posted to GitHub. Want me to apply the mechanical cleanups (indentation, the bool skip= de-duplication, <=> handling) on claude/review-94021abc-ddfigb, or leave the WIP as-is?

The uncommitted change is from my git checkout of the file at the target commit during review. Let me restore the working tree.

The working tree is clean again. That change was just a temporary side effect of checking out the file at the target commit to read it during the review — it wasn't a real code change, so I've restored it rather than committing it. Nothing to push.

The review above stands as the deliverable. Let me know if you'd like me to apply the mechanical cleanups (indentation fix, bool skip= de-duplication, <=> handling) on claude/review-94021abc-ddfigb.