Skip to content

Keep the field indexes two record types agree on when taking their maximum - #4631

Open
g31pranjal wants to merge 2 commits into
mainfrom
apple/g31pranjal/unnested/preserve-record-field-indexes
Open

g31pranjal wants to merge 2 commits into
mainfrom
apple/g31pranjal/unnested/preserve-record-field-indexes

Conversation

@g31pranjal

@g31pranjal g31pranjal commented Sep 17, 2026

Copy link
Copy Markdown
Member

A record type read off a descriptor carries the descriptor's field numbers, and those numbers may skip. Type.Record.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.

Type.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, then routes a value into whichever field happens to carry the same number in the renumbered descriptor. Concretely, for a table whose proto is numbered first_name=1, <2 skipped>, last_name=3, id=4, copying between the two descriptors writes last_name (source 3) into id (target 3) and fails with:

IllegalArgumentException: Wrong object type used with protocol message reflection.
Field number: 3, field java type: LONG, value type: java.lang.String

This fix carries the index over whenever both sides agree on it, by the same rule already used for the field name. Indexes are kept only when all of them survive, since normalizeFields() renumbers every field as soon as a single one is missing an index.

This matters for today in AbstractArrayConstructorValue.resolveElementType() (the element type of a multi-row VALUES list).

Note the defect is latent on main. The issue is only surfaced in case of EXPLODE result wrapped in RCV. Currently, wrapping in RCV happens for WITH ORDINAL case, but not without out. As I make RCV wrapping default in #4623 , skipped-field-number-proto.yamsql fails owing to reason described above.

@g31pranjal g31pranjal added the bug fix Change that fixes a bug label Sep 17, 2026
@g31pranjal
g31pranjal added this pull request to stack #4632 September 17, 2026 22:35
@g31pranjal
g31pranjal removed this pull request from stack #4632 September 17, 2026 22:39
@g31pranjal
g31pranjal force-pushed the apple/g31pranjal/unnested/explode-ordinality-matching branch from 2979d09 to c781f27 Compare September 17, 2026 22:39
@g31pranjal
g31pranjal force-pushed the apple/g31pranjal/unnested/preserve-record-field-indexes branch from 2fe1f19 to 10e1538 Compare September 17, 2026 22:39
@g31pranjal
g31pranjal added this pull request to stack #4634 September 17, 2026 22:44
@g31pranjal
g31pranjal removed this pull request from stack #4634 September 17, 2026 22:48
…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.
@g31pranjal
g31pranjal changed the base branch from apple/g31pranjal/unnested/explode-ordinality-matching to main September 18, 2026 10:58
@g31pranjal
g31pranjal force-pushed the apple/g31pranjal/unnested/preserve-record-field-indexes branch from 10e1538 to 800e6e0 Compare September 18, 2026 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug fix Change that fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant