Core: Fix reference to nested column added to map/list in same transaction#17034
Open
huan233usc wants to merge 1 commit into
Open
Core: Fix reference to nested column added to map/list in same transaction#17034huan233usc wants to merge 1 commit into
huan233usc wants to merge 1 commit into
Conversation
1e3f11d to
2c14b2e
Compare
ebyhr
reviewed
Jul 2, 2026
Comment on lines
+171
to
+177
| // fullName uses the parent's canonical name, which for a map value or list element | ||
| // struct includes the synthetic "value"/"element" segment (e.g. locations.value.alt). | ||
| // Also index the user-facing short name (locations.alt) so later operations in this | ||
| // transaction that reference the column by that name resolve it. | ||
| // Add the short name only if it is not already mapped, so a name containing dots | ||
| // cannot shadow another added column that already owns that name (as its canonical | ||
| // full name or its own short name). |
Member
There was a problem hiding this comment.
This comment is too verbose. I suggest simplifying it. The same goes for the new tests.
Contributor
Author
There was a problem hiding this comment.
Thanks, simplified a bit
…ction SchemaUpdate records an added column in addedNameToId using the parent's canonical name, which for a map value or list element struct includes the synthetic "value"/"element" segment (e.g. locations.value.alt). But callers address such a column by its user-facing short name (locations.alt), so a later operation in the same transaction -- updateColumn, updateColumnDoc, requireColumn, makeColumnOptional, or a move -- fails to resolve it with "Cannot update missing column". Also index the short name in addedNameToId so the shorthand resolves, matching how the schema's own name index (IndexByName) tracks both the full and short paths for map value / list element structs.
2c14b2e to
2561fea
Compare
wombatu-kun
approved these changes
Jul 2, 2026
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.
Problem
SchemaUpdaterecords a column added in the current transaction inaddedNameToId, keyed by the column's name, so that a later operation in the same transaction (updateColumn,updateColumnDoc,requireColumn,makeColumnOptional, or a move) can resolve the not-yet-committed column.For a column added into a map value struct or a list element struct,
internalAddColumnbuilds the key from the parent's canonical name viaschema.findColumnName(parentId), which includes the syntheticvalue/elementsegment:But callers address such a column by its user-facing short name (
locations.alt,points.z) — the form the schema's own name index exposes for map value / list element structs.findForUpdatefirst triesfindField(name)(which can't see the pending column) and then falls back toaddedNameToId.get(name), so the lookup misses and the operation fails withCannot update missing column: locations.alt.Only the full canonical path (
locations.value.alt) works today, which is not the name users write.Example
Fix
Also index the user-facing short name in
addedNameToIdfor nested adds, so the shorthand resolves. This mirrors how the schema's ownIndexByNametracks both the full and short paths (droppingvalue/elementfor structs nested in a map value / list element). Top-level and plain-struct adds are unaffected because their short name equals the full name.Tests
Added
TestSchemaUpdate.testUpdateColumnAddedToMapValueOrListElementInSameTransaction: adds a field into a map value struct and a list element struct, then references each by its short name (locations.alt,points.z) in the same transaction. It fails before the change withCannot update missing columnand passes after. The fullTestSchemaUpdatesuite remains green.