MDEV-40486 Length check for vector fields in CREATE TABLE ... SELECT

[mariadb-YuchenPei]

The changes of MDEV-39558 2b65294 added length check assertion in Field_varstring constructors, and length check in type inference for SELECT set operations, to emit errors before reaching the assertions.

That change caused an error to turn into an assertion failure in a separate path, when the length limit violation is not detected before tripping the assertion. So in this patch we fix it by adding an earlier length check in that path.

[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-40486 — Length check for vector fields in CREATE TABLE ... SELECT

,Commit: 3b1915be · 3 files, +27/−2 · base 11.8

Overview

The commit fixes an assertion failure (regression from MDEV-39558) that occurs when CREATE TABLE ... SELECT VEC_FROMTEXT(...) produces a vector column whose computed length exceeds MAX_FIELD_VARCHARLENGTH. MDEV-39558 added a length assertion in the Field_varstring constructor plus a pre-emptive error in the set-operations type-inference path — but the plain CREATE ... SELECT path had no such guard, so it tripped the assertion instead of raising a clean error. Two changes:

1. item_vectorfunc.h — create_field_for_create_select() now checks max_length > MAX_FIELD_VARCHARLENGTH and raises ER_TOO_BIG_FIELDLENGTH (returning NULL) before the field is constructed.
2. item_vectorfunc.cc — fix_length_and_dec() now sizes the result from max_char_length() rather than max_length (bytes), and writes the multiplier as sizeof(float) / 2.

Correctness — solid

• The core fix is right and safe. I verified the caller at sql_insert.cc:4921 (create_table_from_items): it checks if (!tmp_field) DBUG_RETURN(NULL), so returning NULL after my_error() propagates the error cleanly — no NULL-deref, no crash. This is the standard failure contract for create_field_for_create_select.
• The error is consistent with the existing convention. sql/sql_type_vector.cc:89-93 (the vector column-definition check) uses the identical my_error(ER_TOO_BIG_FIELDLENGTH, MYF(0), <name>, static_cast<ulong>(MAX_FIELD_VARCHARLENGTH / sizeof(float))). The new code mirrors it exactly, and the reported max = 16383 (65532 / 4) is coherent with that path's messaging (max = number of floats).
• > (not >=) is the correct boundary — max_length == MAX_FIELD_VARCHARLENGTH (65532) is exactly the largest a varstring can hold, matching the sql_type_vector.cc guard.
• No buffer risk from the .cc change. I checked val_str(): it builds the result with buf->append(), which reallocates dynamically. The max_length value is used only for field/type sizing, never as a fixed buffer bound — so a smaller estimate cannot cause an overflow.

The max_char_length() change — mostly an improvement, one edge worth noting

Switching from max_length (bytes) to max_char_length() is genuinely more accurate for the dominant case: vector text is ASCII, so the float count is bounded by characters, not bytes. For a char-limited source like VARCHAR(16383) CHARACTER SET utf8mb4, the old formula computed (65532−1)*2 ≈ 131062 and spuriously errored; the new formula gives (16383−1)*2 = 32764, correctly allowing it. Good.

The one wrinkle: max_char_length() is a guaranteed-capacity figure (max_length / mbmaxlen), not a true upper bound on character count for byte-limited types. A TEXT/BLOB column with a multi-byte charset (e.g. utf8mb4 TEXT, max_char_length() == 16383) can physically hold up to 65535 ASCII characters, i.e. up to ~32767 floats. The new formula would size the resulting VECTOR field for only ~8191. This is not a crash or overflow (dynamic append, and over-length storage would surface as a runtime "data too long", not silent corruption), and the added test uses a single-byte charset so it's unaffected — but it's a behavioral corner the old byte-based formula covered conservatively. Worth a mental note, or a comment, if multi-byte TEXT sources are a realistic input.

Style / conventions — minor

• Stale comment. The comment block at item_vectorfunc.cc:190-193 still reads "(max_length - 1) / 2 floats … (max_length - 1) * 2", but the code now uses max_char_length() and sizeof(float) / 2. Please update it to match, otherwise the derivation no longer describes the code.
• sizeof(float) / 2 is a touch obscure. It evaluates to 2 and makes the "4 bytes per float" explicit, but the / 2 (two characters per number: a digit + a comma) is still an unexplained magic constant. A one-line comment like /* sizeof(float) bytes per float, min 2 chars ("d,") per float */ would make the intent self-evident.

Pre-existing edge (not introduced here, but exposed)

If args[0]->max_char_length() is 0 (e.g. VEC_FROMTEXT('')), (0 - 1) underflows the unsigned arithmetic to a huge value, which then trips the new > MAX_FIELD_VARCHARLENGTH check and yields a confusing ER_TOO_BIG_FIELDLENGTH for empty input. The old code had the same underflow (and in this path would have hit the assertion), so the patch is strictly an improvement — but a max_char_length() ? ... : 0 guard would be more robust if you want to close it.

Test coverage — adequate

vector2.test adds the exact failing repro and asserts ER_TOO_BIG_FIELDLENGTH, which pins the assertion→error fix. Two optional additions would strengthen it: (a) a positive case with a VARCHAR(16383) CHARACTER SET utf8mb4 source that now succeeds (the case the max_char_length() change specifically fixes), and (b) a boundary case right at the 16383-float limit.

Verdict

Correct, minimal, and idiomatic — it resolves the assertion the right way and matches the established ER_TOO_BIG_FIELDLENGTH pattern. No blocking issues. Recommended before merge: update the now-stale comment in fix_length_and_dec(); optionally add the positive/boundary tests and consider the multi-byte TEXT sizing note above.

[mariadb-YuchenPei]

claude:

---

Code Review: MDEV-40486 (revised) — Length check for vector fields in CREATE TABLE ... SELECT

Commit: 4e97c43 · 4 files, +73/−4 · base 11.8

Overview

Revised version of the earlier 3b1915be. Fixes a regression from MDEV-39558 where an over-length vector column in CREATE TABLE ... SELECT VEC_FROMTEXT(...) tripped a Field_varstring assertion instead of raising a clean error. Three code effects:

1. item_vectorfunc.h — create_field_for_create_select() now raises ER_TOO_BIG_FIELDLENGTH (returning NULL) when max_length > MAX_FIELD_VARCHARLENGTH, before the field is built.
2. item_vectorfunc.cc — length estimate now derives from max_char_length() (character count) rather than max_length (bytes), and guards the empty-string case: maxlen ? (maxlen - 1) * 2 : 0.
3. vector2.test/result — four scenarios added.

This version resolves all the feedback from the prior round: max_char_length() (which correctly strips the mbmaxlen inflation), the comment updated to match, the obscure sizeof(float)/2 reverted to * 2, and the empty-string underflow closed.

Correctness — solid

• Underflow guard is right and verified. maxlen ? (maxlen - 1) * 2 : 0 prevents the (0 - 1) unsigned wrap. The empty-string test confirms it: with the old code, VEC_FROMTEXT('') would wrap to a huge length and wrongly raise ER_TOO_BIG_FIELDLENGTH; now max_length is 0, the guard is skipped, and the JSON parser correctly produces ER_JSON_EOS at copy time.
• Guard boundary is correct. > (not >=) matches the varstring capacity, and the reported max = 16383 (MAX_FIELD_VARCHARLENGTH / sizeof(float) = 65532 / 4) is consistent with the existing vector column-def check at sql_type_vector.cc:89-93. The NULL return is handled by the caller (sql_insert.cc:4921), so no crash.
• Boundary tests are precise and valuable. 16383-dim → (32767-1)*2 = 65532 = MAX_FIELD_VARCHARLENGTH → succeeds; 16384-dim → 65536 → error. That pins the exact max-dimension edge, which is the most useful regression to have.

Test coverage — one meaningful gap

• The headline change (max_length → max_char_length()) is not actually covered by any test. All non-empty tests run under the single-byte default charset, where max_char_length() == max_length, so they'd pass identically with the old byte-based formula. I verified this: reverting just that line (keeping the maxlen ? ... : 0 guard) leaves every added test green. Only the empty-string test distinguishes new from old.
• Recommendation: add a multi-byte case, which is exactly where the two formulas diverge — e.g. a VARCHAR(16383) CHARACTER SET utf8mb4 source producing an 8191-dim vector. Under the new code it succeeds ((16383-1)*2 = 32764); under the old max_length-based code it would be (65532-1)*2 = 131062 → spurious ER_TOO_BIG_FIELDLENGTH. That single test is what protects the change you actually made. (TEXT/BLOB won't work as a witness — Field_blob::char_length() returns the byte capacity, so max_char_length() is 65535 for any charset.)

Minor notes

• Transient zero-length field for ''. With max_length == 0 the guard is skipped and create_table_field_from_handler() builds a 0-length (vector(0)) field before the ER_JSON_EOS fires at data copy. It's benign here because the JSON error aborts the statement first, but vector(0) is otherwise rejected (ER_WRONG_FIELD_SPEC) in the column-definition path — worth being aware of if the copy-time error path ever changes.
• Style: uint maxlen is fine; max_char_length() returns uint32, so no narrowing concern for any realistic type.

Verdict

Correct, well-scoped, idiomatic, and it cleanly incorporates the earlier feedback. No blocking issues. The one thing I'd genuinely push for before merge is a utf8mb4 VARCHAR test so the max_char_length() change is regression-protected; the rest are optional.

[mariadb-YuchenPei]

re claude comment

The headline change (max_length → max_char_length()) is not actually covered by any test.

This is actually covered, for example, in this testcase:

CREATE TABLE t (a VARCHAR(16383)) AS
  SELECT concat('[1', repeat(',1', 8190), ']') AS a;
CREATE TABLE tt AS SELECT VEC_FROMTEXT(a) AS f FROM t;
DROP TABLE t,tt;

Before the change from max_length to max_char_length() this would fail with ER_TOO_BIG_FIELDLENGTH, because the max_length of VARCHAR(16383) is 65532 which results in 131064 max length for the vector, exceeding its limit. After the change it uses max_char_length() which is, well, 16383 and is correct.

Recommendation: add a multi-byte case, which is exactly where the two formulas diverge — e.g. a VARCHAR(16383) CHARACTER SET utf8mb4 source producing an 8191-dim vector.

I am not sure how this is possible, so I asked claude to come up with a testcase and it came up with a wrong one. After more back and forth it conceded this point is invalid. The second round review is done after that, but somehow clauded decided to make the same comment again