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

[sanja-byelkin]

Main problem is spoiled SHOW CREATE VIEW (or I just do not understand how it should be shown)

CREATE TABLE t1 (a CHAR(1));
CREATE TABLE t2 AS SELECT VEC_FROMTEXT(a) AS f FROM t1;
SHOW CREATE TABLE t2;
Table	Create Table
t2	CREATE TABLE `t2` (
  `f` vector(0) DEFAULT NULL
) ENGINE=MyISAM DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci
INSERT INTO t2 VALUES (VEC_FROMTEXT('[1]'));
ERROR 22007: Incorrect vector value: '\x00\x00\x80?' for column `test`.`t2`.`f` at row 1
CREATE TABLE t3 (f VECTOR(0));
ERROR 42000: Incorrect column specifier for column 'f'
DROP TABLE t1, t2;

i.e. it shows impossible CREATE TABLE

here is a test case (of course it is incorrect just changed to pass):

CREATE TABLE t1 (a CHAR(1));
CREATE TABLE t2 AS SELECT VEC_FROMTEXT(a) AS f FROM t1;
SHOW CREATE TABLE t2; #  `f` vector(0)
--error ER_TRUNCATED_WRONG_VALUE
INSERT INTO t2 VALUES (VEC_FROMTEXT('[1]'));  # rejected, count(*) = 0

--error ER_WRONG_FIELD_SPEC
CREATE TABLE t3 (f VECTOR(0));

DROP TABLE t1, t2;

Add SHOW CREATE TABLE to the test and fix it please. Also there are some quietsions

[mariadb-YuchenPei]

Main problem is spoiled SHOW CREATE VIEW (or I just do not understand how it should be shown)

Discussed with @vuvova and understand that zero-dimension vectors should not be allowed in table fields, even though select (VEC_FROMTEXT('[]')); is currently ok. So updated the patch accordingly.

[mariadb-YuchenPei]

claude:

---

Code Review: MDEV-40486 (iteration 3) — Length check for vector fields

Commit: 89b5f31 · 3 files, +111/−5 · base 11.8

Overview

Third revision. The length validation is relocated from the CREATE-only field-creation hook into Item_func_vec_fromtext::fix_length_and_dec(), and now covers two conditions raised as hard errors during query resolution:

• max_length == 0 (input too short to ever be a valid vector) → ER_TRUNCATED_WRONG_VALUE
• max_length > MAX_FIELD_VARCHARLENGTH → ER_TOO_BIG_FIELDLENGTH

Plus expanded tests (empty vector, zero-length, CHAR(1), VECTOR(0), view creation) and a rewrite of the pre-existing MDEV-35141 test.

Main concern — the scope change is an incompatible behavior change

Because the check now fires in fix_length_and_dec based on the argument's static worst-case max_length, VEC_FROMTEXT(<wide expression>) is rejected everywhere — regardless of the actual runtime data or whether a new column is even being created.

• The clearest evidence is in this very diff: the existing MDEV-35141 line had to be rewritten —

  -insert into t1 select vec_fromtext(concat('[',group_concat(1),']')) from seq_1_to_64;
  +insert into t1 select vec_fromtext(cast(concat('[',group_concat(1),']') as char(130))) ...
  

  Here t1.v is already vector(64) and the actual value is a valid 64-dim vector. GROUP_CONCAT carries max_length = group_concat_max_len (default 1 MB), so concat(...) is a mediumblob-typed expression → (maxlen-1)*2 ≈ 2 M > 65532 → ER_TOO_BIG_FIELDLENGTH. The statement now fails at resolution even though it's a plain INSERT ... SELECT into a correctly-sized existing column that previously worked. The CAST(... AS CHAR(130)) is a workaround, not a fix.
• The previous (create_field_for_create_select) approach did not have this problem — it only triggered when materializing a new column via CREATE ... SELECT, matching the bug's actual scope. The new approach trades that precision for uniform coverage (views, set-ops, plain selects) at the cost of rejecting legitimate INSERT/SELECT/CREATE VIEW uses of wide-typed inputs.

This may be an intentional, defensible policy (a VEC_FROMTEXT result that could exceed any vector column's capacity is arguably always an error). But it is an incompatible change: users building vectors from GROUP_CONCAT/CONCAT/blob expressions must now add casts. Recommend: confirm this is intended, call it out explicitly in the MDEV/commit message as an incompatible change, and add a one-line comment on the modified MDEV-35141 test explaining why the cast is now required (otherwise it looks arbitrary to future maintainers).

Secondary risks

• Prepared statements / placeholders. VEC_FROMTEXT(?) — an Item_param typically has max_char_length() == 0 at PREPARE, so fix_length_and_dec would hit max_length == 0 → ER_TRUNCATED_WRONG_VALUE and fail the PREPARE, where before it resolved and got its real length at EXECUTE. I haven't confirmed this empirically, but it's plausible and there's no test for it — please verify PREPARE ... VEC_FROMTEXT(?) still works and add a regression test.
• Plain SELECT behavior. SELECT VEC_FROMTEXT(char1col) FROM t now errors at resolution instead of returning per-row NULL + warning. Degenerate, but a change worth being deliberate about.

Code quality / message

• Misleading error text. The max_length == 0 branch passes name.str into the value slot: my_error(ER_TRUNCATED_WRONG_VALUE, MYF(0), "vector", name.str) renders as Truncated incorrect vector value: 'f', where f is the column alias, not a value. A reader will think the value "f" was the offender. The ER_TOO_BIG_FIELDLENGTH branch uses name.str correctly (its slot is a column name). Consider an error that doesn't put the column name in a value slot.
• name.str availability. create_field_for_create_select ran after item naming was settled; fix_length_and_dec runs earlier during fix_fields. The recorded outputs show name.str populated, but please confirm it's always non-null here (e.g. for un-aliased/nested contexts) so my_error("%s", name.str) can't get a null.
• The underflow guard maxlen ? (maxlen - 1) * 2 : 0, the max_char_length() switch, and the updated comment are all correct and clean.

Test coverage

• Good additions: the 16383-ok / 16384-error boundary, '[]', '', CHAR(1), VECTOR(0), and the view case.
• Still no multi-byte charset test. Every test runs under the single-byte default, where max_char_length() == max_length, so the headline max_length → max_char_length() change remains unverified by any regression test — reverting just that token would keep all tests green. Add a VARCHAR(16383) CHARACTER SET utf8mb4 case (succeeds now, would spuriously ER_TOO_BIG_FIELDLENGTH under the old byte-based formula).
• Add the prepared-statement case noted above.

Verdict

The mechanics (underflow guard, max_char_length(), boundary tests) are correct and the coverage is much broader than before. The open question is deliberate scope: moving the check into fix_length_and_dec rejects previously-valid INSERT/view/SELECT queries over wide-typed inputs — the rewritten MDEV-35141 test is proof it changes existing behavior. Before merge I'd want (1) confirmation that this broadening is intended and documented as incompatible, (2) the prepared-statement path verified with a test, and (3) the ER_TRUNCATED_WRONG_VALUE argument reconsidered. The multi-byte test remains the one coverage gap I'd insist on.

[mariadb-YuchenPei]

The length validation is relocated from the CREATE-only field-creation hook into Item_func_vec_fromtext::fix_length_and_dec()

This is following @abarkov 's review comments. Updated commit message to explain this change.

[mariadb-YuchenPei]

Amended the commit addressing more review comments by @abarkov

[abarkov]

The patch is OK to push. Thanks.