Merging struct types renumbers fields and drops the type name (for array literals, COALESCE) #4734

An array literal with two or more elements of a struct type, such as SELECT [t.s, t.s] FROM T, triggers an error at run time when the field numbers of the protobuf message of the struct are not exactly 1..n in declaration order. The query fails with an exception from protobuf reflection. If the types happen to line up, we expect that the query might copy values into the wrong fields with no error.

Reproduction

Use record metadata from a hand-written .proto file. (A struct defined with CREATE TYPE AS STRUCT is generally not affected, as the fields will be numbered 1..n in declaration order.)

message Ref {
  enum Kind {
    A = 1;
    B = 2;
  }
  optional Kind kind = 2;    // numbering starts at 2
  optional string value = 3;
}

message T {
  optional int64 id = 1;
  optional Ref r = 2;
}

Save a record of type T with r set (and r.kind set), then run:

SELECT [t.r, t.r] AS rs FROM T AS t

Expected: one row whose rs is an array of two Ref values.

Actual:

java.lang.UnsupportedOperationException: This field is not of enum type. (__type__9bfeeb6f_fb08_441a_94f6_29ee4c816307.value)
    at com.google.protobuf.Descriptors$FieldDescriptor.getEnumType(Descriptors.java:1337)
    at ...cascades.values.MessageHelpers.deepCopyMessage(MessageHelpers.java:290)
    at ...cascades.values.MessageHelpers.deepCopyMessageIfNeeded(MessageHelpers.java:253)
    at ...cascades.values.RecordConstructorValue.deepCopyIfNeeded(RecordConstructorValue.java:210)
    at ...cascades.values.RecordConstructorValue.deepCopyIfNeeded(RecordConstructorValue.java:196)
    at ...cascades.values.RecordConstructorValue.eval(RecordConstructorValue.java:123)
    at ...plans.RecordQueryMapPlan.lambda$executePlan$0(RecordQueryMapPlan.java:95)

A gap in the numbering can similarly cause an exception. For example, with int64 a = 1; int64 b = 3; string c = 4;, the value of b goes to the merged field 3, which is the string c:

java.lang.IllegalArgumentException: Wrong object type used with protocol message reflection.
Field number: 3, field java type: STRING, value type: java.lang.Long
    at com.google.protobuf.FieldSet$Builder.verifyType(FieldSet.java:1232)
    ...
    at ...cascades.values.MessageHelpers.deepCopyMessage(MessageHelpers.java:292)

Cause

1. The type of an array literal is the maximum type of its elements. For two record types, Type.maximumType() builds each merged field with Record.Field.of(resultFieldType, resultFieldNameOptional). That call sets no field index. maximumType() also has no early return when both types are equal.
2. Type.Record.fromFields() calls normalizeFields(), which gives each field without an index the number ordinal + 1. So the merged type numbers its fields 1..n by position. The merged record also has no name, so it gets a generated name from ProtoUtils.uniqueTypeName().
3. When RecordConstructorValue.eval() builds the result row, MessageHelpers.deepCopyMessage() copies each element into the merged descriptor. It finds the target field with findFieldByNumber(field.getNumber()). Its Javadoc assumes that the target is "wire-compatible" with the source. But after step 2, the target is not wire-compatible.

Impact

• The query fails for any array literal of structs whose field numbers are not 1..n in declaration order.
• There could a danger of silent wrong results if the field numbers are a permutation of 1..n and the moved fields have the same types. For example, int64 b = 2; int64 a = 1; (declared in this order) could swap the values of a and b. (To be tested/confirmed.)
• The merged type needlessly gets a generated name assigned even when all elements have the same named type. A client that is sensitive to the struct type name then wouldn’t be able to recognize the elements.

Suggested fix

• In the RECORD case of Type.maximumType(), keep the field index (and the storage name) when both fields agree, so that the merged descriptor stays wire-compatible.
• If both types are equal apart from nullability, return the first type with the merged nullability. Then the merged type will also keep its name.