Repository navigation
Conversation
adsharma
force-pushed
the
chore/setvalue-size-guard
branch
from
October 29, 2026 18:20
fc7f55c to
f494320
Compare
UnionVector::setTagField stored the tag as a uint16 in the union vector's own data, which holds each row's struct_entry_t position in the field vectors. For an unfiltered chunk of n rows it zeroed the positions of rows 1 to ceil(n/4) - 1, and the result vector is reused, so later chunks kept the damage. Everything that follows those positions read position 0: list literal, list_append and map copies, CAST to STRING and comparisons. [union_value(a := i)] returned [1] for i = 2, and 1,022 of 3,000 rows held another row's value. union_extract and a plain RETURN read the field vectors at the row's own position and were right. The tag now goes into the tag vector, as the cast and JSON paths do. union_value builds a one-member union, so the tag is 0, which the never-written tag vector already returned from its zero-filled buffer. union_value names that member index instead of passing UnionType::TAG_FIELD_IDX, the tag's position among the struct fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ValueVector::setValue<T> indexes the value buffer as an array of T, so a T of another size lands in the wrong slots without an error, as union_value's tags did. It now asserts that sizeof(T) is the vector's value size, which throws in runtime-check builds. CompressChunkTest.TestDoubleInPlaceUpdateNoExceptions tripped it: it filled a FLOAT vector with doubles for a DOUBLE chunk, and now uses a DOUBLE vector. The fts extension trips it as well, by writing a new term's df as an int into a UINT64 vector, so the extension fix and its submodule bump have to land first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zahariash
force-pushed
the
chore/setvalue-size-guard
branch
from
November 2, 2026 00:09
f494320 to
e94b26b
Compare
zahariash
marked this pull request as ready for review
November 11, 2026 12:29
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This work was produced with the help of language models.
Stacked on #1463; only the last commit is new. Depends on LadybugDB/extensions#141: the
minimal linux extension testjob fails on the new assertion until that fix merges and theextensionsubmodule includes it. The submodule bump is added here as a commit once it merges, and CI is re-run then.ValueVector::setValue<T>indexes the value buffer as an array ofT, so aTof another size writes into the wrong slots without an error. That is howunion_valuedamaged its rows' positions in #1425: 2-byte tags went into a vector of 8-byte positions.setValuenow asserts thatsizeof(T)is the vector's value size. Release builds compile it out unlessRUNTIME_CHECKSis set.Callers.
CompressChunkTest.TestDoubleInPlaceUpdateNoExceptionsfilled aFLOATvector with doubles for aDOUBLEchunk and tripped it; it now uses aDOUBLEvector. The fts extension wrote a new term's df as anintinto aUINT64vector. No other engine test trips it.Tests.
value_vector_test.cppchecks that a 2-byte write into a union vector throws and keeps the row's position, that anINT64write into anINT32vector throws, and that matching writes go through. Withmain'sunion_value,union.testfails on the assertion. Without LadybugDB/extensions#141, the fts e2e tests fail 11 cases on the assertion, and the half-applied inserts then crash a later query, a separate exception-safety problem. With it, 86 of the 87 pass;show_loaded_extensionsalso needs the duckdb extension, which the run did not build. Full ctest: 3229/3229.