Repository navigation
Match fields by position rather than field number when deep-copying messages - #4631
Merged
g31pranjal merged 8 commits intoNov 2, 2026
Merged
Conversation
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/explode-ordinality-matching
branch
from
September 17, 2026 22:39
2979d09 to
c781f27
Compare
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
from
September 17, 2026 22:39
2fe1f19 to
10e1538
Compare
g31pranjal
changed the base branch from
apple/g31pranjal/unnested/explode-ordinality-matching
to
main
September 18, 2026 10:58
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
from
September 18, 2026 10:59
10e1538 to
800e6e0
Compare
g31pranjal
marked this pull request as ready for review
September 18, 2026 11:48
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
from
October 4, 2026 21:38
ea52626 to
7f4ede7
Compare
…ximum A record type read off a descriptor carries the descriptor's field numbers, and those numbers may skip. `defineProtoType()` writes them back out as the field numbers of the descriptor it generates for the type, so two types that disagree about them generate descriptors that are not wire-compatible with each other. `maximumType()` rebuilt each field with `Field.of(type, name)`, which has no index, so `normalizeFields()` renumbered every field by position. The maximum of two record types that agree on their numbering therefore lost that numbering, and `MessageHelpers.deepCopyMessage()`, which relates the fields of two descriptors by number, would then route a value into the field that happens to carry the same number in the renumbered descriptor -- writing, for instance, a string into a field declared `int64`. Carry the index over whenever both sides agree on it, by the same rule already used for the field name. The indexes are only kept when all of them survive, since `normalizeFields()` renumbers all fields as soon as a single one is missing.
…ld number A record type read off a descriptor carries the descriptor's field numbers, and those numbers may skip. Two descriptors generated for record types that agree on everything but the field numbers therefore number the same field differently. `MessageHelpers.deepCopyMessage()` relates the fields of two descriptors by number, so copying a message between such descriptors puts a value into whichever field happens to carry its number on the other side -- writing, for instance, a string into a field declared `int64`. Instead of keeping the field numbers alive through `Type.maximumType()`, coerce by position: `coerceObject()` without any promotions now copies a message through the new `coerceMessageByIndexIfNeeded()`, which relates the fields of the two descriptors by their position, just as `coerceMessage()` already does when there are promotions to apply.
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
from
November 2, 2026 00:32
7f4ede7 to
6f6111a
Compare
g31pranjal
changed the title
…essages Two descriptors of the same structure need not number its fields the same way, e.g. if one of them keeps the numbers of a proto that skips some while the other is numbered by position. `MessageHelpers.deepCopyMessage()` matched the fields of the two descriptors by number, so a copy between such descriptors put a value into whichever field happened to carry its number on the other side -- writing, for instance, a string into a field declared `int64`. Match the fields by position instead, which leaves the copy unchanged wherever the numbering already agrees. The target descriptor may have additional trailing fields, such as the pseudo-fields of a record type. A nested message is now also copied into the target's message type rather than set as is. This replaces the earlier approach of keeping the field indexes alive through `Type.maximumType()` and of coercing by position in `coerceObject()`.
g31pranjal
changed the title
The branch that sets a nested message as is never runs for the DynamicMessage builders deepCopyMessage is used with, as their default instances are never identical, so the merge branch already copies nested messages into the target's message type.
normen662
requested changes
Nov 2, 2026
| // find the field on the target side | ||
| final var targetField = builder.getDescriptorForType().findFieldByNumber(field.getNumber()); | ||
| // find the field on the target side, by position | ||
| final var targetFields = builder.getDescriptorForType().getFields(); |
Contributor
There was a problem hiding this comment.
can you move this out of the loop
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
from
November 2, 2026 16:14
8876285 to
f41988e
Compare
normen662
approved these changes
Nov 2, 2026
g31pranjal
deleted the
apple/g31pranjal/unnested/preserve-record-field-indexes
branch
November 2, 2026 17:00
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.
Makes
MessageHelpers.deepCopyMessage()match the fields of the source and target descriptors by position rather than by field number.Problem
A record type read off a descriptor keeps the descriptor's field numbers, which may skip, while a type built up field by field (e.g. by
Type.maximumType()for the element type of a multi-rowVALUESlist) is numbered by position. Two descriptors of the same structure can therefore number the same field differently, and a copy that matches fields by number writes a value into the wrong field. For a table whose proto is numberedfirst_name=1, <2 skipped>, last_name=3, id=4,last_nameends up inid:Changes
deepCopyMessage()matches fields by position. The target descriptor may have additional trailing fields, such as the pseudo-fields of a record type.MessageHelperTestscover nested records, enums, repeated and nullable repeated records, and a target with pseudo-fields.The defect is latent on
main. It shows up once every explode wraps its elements in a record constructor, as in #4816, whereskipped-field-number-proto.yamsqlandfield-index-tests-proto.yamsqlfail without this fix.