From f051ae27fb221d40d416bcb2f62ba04d8135e030 Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Fri, 9 Oct 2026 14:49:47 +0100 Subject: [PATCH] Make `MetaDataProtoEditor` always rename the canonically named union field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When several union fields reference a renamed record type, `renameRecordType()` and `renameRecordTypes()` now always rename the one that is canonically named. Previously, a field with a higher number could “displace” it, which would result in the canonical field keeping its old name. This could also cause a batch rename that swaps two type names to be spuriously rejected with “Cannot rename union field because a field of the new name already exists”. `RecordMetaDataBuilder` writes the records of a type under its canonically named union field if there is one, and otherwise under the one with the highest number. So the old behavior also effectively moved the records of the renamed type to the higher-numbered field. With this change, the editor now selects the union field by the same rule as `RecordMetaDataBuilder`, so that records keep getting written under the original field. Follow-up to #4536. Resolves #4732. --- .../foundationdb/MetaDataProtoEditor.java | 8 +- .../MetaDataProtoEditorUnitTest.java | 116 ++++++++++++++++-- 2 files changed, 109 insertions(+), 15 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java index a17c7529c9..7ff7035075 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java @@ -972,11 +972,13 @@ private static void determineRecordTypeUnionFieldsAndUsages( continue; } - // If multiple fields reference this record type, prefer the canonically-named one; otherwise, keep the - // one with the highest field number. + // If multiple fields reference this record type, prefer the canonically named one; otherwise, keep the + // one with the highest field number. This is the same rule that `RecordMetaDataBuilder.remapUnionField()` + // uses to choose the union field that records of the type are written under. The two must be kept in sync. if (rename.unionField == null || rename.canonicalFieldName.equals(unionField.getName()) - || unionField.getNumber() > rename.unionField.getNumber()) { + || (!rename.canonicalFieldName.equals(rename.unionField.getName()) + && unionField.getNumber() > rename.unionField.getNumber())) { rename.unionField = unionField; } } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java index aa3a554e2f..adeefd9d41 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java @@ -577,6 +577,24 @@ public static Stream renamableFiles() { )); } + /** + * Adds a field named {@code fieldName} with number {@code number} to the union message type in + * {@code MetaData.records}, referencing the message type {@code typeName}. + */ + private static void addUnionField(@Nonnull RecordMetaDataProto.MetaData.Builder builder, + @Nonnull String fieldName, int number, @Nonnull String typeName) { + for (final DescriptorProtos.DescriptorProto.Builder messageType : + builder.getRecordsBuilder().getMessageTypeBuilderList()) { + if (messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + messageType.addField(DescriptorProtos.FieldDescriptorProto.newBuilder() + .setName(fieldName) + .setNumber(number) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName(typeName)); + } + } + } + @Nonnull private static Descriptors.Descriptor getMessage(final RecordMetaData renamed, final String T1) { return renamed.getRecordsDescriptor().getMessageTypes().stream() @@ -656,6 +674,33 @@ void renameRecordTypesAllowsSwappingTwoNames() throws IOException { assertEquals(3, renamed.getRecordType("UUID").getDescriptor().getFields().size()); } + /** + * Tests that a batch that swaps two names must succeed even when a higher-numbered, non-canonically named union + * field also references one of the types. In the fixture, {@code _T1} (number 1) and {@code _T2} (number 2) are + * canonical, while {@code T1_v2} (number 3) also references {@code T1}. The canonical fields swap their names, and + * records of each type are still written under the same field number. + */ + @Test + void renameRecordTypesAllowsSwappingTwoNamesWithHigherNumberedField() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + addUnionField(builder, "T1_v2", 3, "T1"); + final RecordMetaDataProto.MetaData originalProto = builder.build(); + final RecordMetaData originalMetaData = RecordMetaData.build(originalProto); + MetaDataProtoEditor.renameRecordTypes(builder, + name -> name.equals("T1") ? "T2" : name.equals("T2") ? "T1" : name, + RecordMetaDataBuilder.getDependencies(originalProto, Map.of())); + final RecordMetaData renamed = RecordMetaData.build(builder.build()); + final Descriptors.Descriptor union = getMessage(renamed, RecordMetaDataBuilder.DEFAULT_UNION_NAME); + // The old T1 is now called T2, and vice versa. Its canonical field followed it, and T1_v2 kept its name. + assertEquals(1, union.findFieldByName("_T2").getNumber()); + assertEquals(2, union.findFieldByName("_T1").getNumber()); + assertSame(renamed.getRecordType("T2").getDescriptor(), union.findFieldByName("T1_v2").getMessageType()); + assertEquals(originalMetaData.getUnionFieldForRecordType(originalMetaData.getRecordType("T1")).getNumber(), + renamed.getUnionFieldForRecordType(renamed.getRecordType("T2")).getNumber()); + assertEquals(originalMetaData.getUnionFieldForRecordType(originalMetaData.getRecordType("T2")).getNumber(), + renamed.getUnionFieldForRecordType(renamed.getRecordType("T1")).getNumber()); + } + /** * Tests that a batch that swaps two names must succeed even when neither type has a canonically named union field, * so that no union field name changes and only their {@code typeName} references are swapped. @@ -728,6 +773,53 @@ void renameUpdatesOnlyTheCanonicalOfSeveralUnionFields() throws IOException { union.getFields().stream().map(Descriptors.FieldDescriptor::getName).collect(Collectors.toSet())); assertSame(renamedT1, union.findFieldByName("_" + simpleRename("T1")).getMessageType()); assertSame(renamedT1, union.findFieldByName("_T1_1").getMessageType()); + // Records of the renamed type are still written under the canonically named field, number 3. + assertEquals(3, renamed.getUnionFieldForRecordType(renamed.getRecordType(simpleRename("T1"))).getNumber()); + } + + /** + * Tests that the canonically named union field is renamed along with its type even when another union field with + * a higher number references the same type. In the fixture, {@code _T1} (number 1) is canonical for {@code T1}, + * while {@code T1_v2} (number 3) also references {@code T1}. + */ + @Test + void renameUpdatesTheCanonicalUnionFieldDespiteHigherNumberedField() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("TwoBoringTypes.json"); + addUnionField(builder, "T1_v2", 3, "T1"); + final RecordMetaData renamed = runRename(builder.build(), + MetaDataProtoEditorUnitTest::simpleRename, + MetaDataProtoEditorUnitTest::simpleRenameUndo); + final Descriptors.Descriptor union = getMessage(renamed, RecordMetaDataBuilder.DEFAULT_UNION_NAME); + final Descriptors.Descriptor renamedT1 = getMessage(renamed, simpleRename("T1")); + assertEquals(Set.of("_" + simpleRename("T1"), "T1_v2", "_" + simpleRename("T2")), + union.getFields().stream().map(Descriptors.FieldDescriptor::getName).collect(Collectors.toSet())); + assertSame(renamedT1, union.findFieldByName("_" + simpleRename("T1")).getMessageType()); + assertSame(renamedT1, union.findFieldByName("T1_v2").getMessageType()); + // Records of the renamed type are still written under the canonically named field, number 1, rather than + // under the higher-numbered T1_v2. + assertEquals(1, renamed.getUnionFieldForRecordType(renamed.getRecordType(simpleRename("T1"))).getNumber()); + } + + /** + * Tests that when several union fields reference the type being renamed and none of them is canonically named, + * none is renamed, and records of the type are still written under the one with the highest number. In the + * fixture, {@code a} (number 1) and {@code a_v2} (number 3) both reference {@code T1}. + */ + @Test + void renameKeepsNonCanonicalUnionFieldsAndTheirHighestNumberedField() throws IOException { + final RecordMetaDataProto.MetaData.Builder builder = loadMetaData("NonCanonicalUnionFields.json"); + addUnionField(builder, "a_v2", 3, "T1"); + final RecordMetaData renamed = runRename(builder.build(), + MetaDataProtoEditorUnitTest::simpleRename, + MetaDataProtoEditorUnitTest::simpleRenameUndo); + final Descriptors.Descriptor union = getMessage(renamed, RecordMetaDataBuilder.DEFAULT_UNION_NAME); + final Descriptors.Descriptor renamedT1 = getMessage(renamed, simpleRename("T1")); + assertEquals(Set.of("a", "a_v2", "b"), + union.getFields().stream().map(Descriptors.FieldDescriptor::getName).collect(Collectors.toSet())); + assertSame(renamedT1, union.findFieldByName("a").getMessageType()); + assertSame(renamedT1, union.findFieldByName("a_v2").getMessageType()); + // Records of the renamed type are still written under the field with the highest number. + assertEquals(3, renamed.getUnionFieldForRecordType(renamed.getRecordType(simpleRename("T1"))).getNumber()); } /** @@ -1582,16 +1674,14 @@ void renameRecordTypesWithConflictingName(boolean t1Conflicts) throws IOExceptio // this is exactly the kind of scenario where one-by-one renaming is *not* expected to match the batched // renaming, so we use `runRenameBatchedOnly()` here rather than `runRename()`. final String prefix = "__Q_"; - final RecordMetaData withConflict = runRenameBatchedOnly(loadMetaData("TwoBoringTypes.json").build(), - oldName -> { - if (t1Conflicts) { - return !oldName.equals("T1") ? prefix + "T1" : oldName; - } else { - return oldName.equals("T1") ? prefix + "T2" : oldName; - } - }, - newName -> newName.startsWith(prefix) ? newName.substring(prefix.length()) : newName); final String conflicting = t1Conflicts ? "T1" : "T2"; + final String other = t1Conflicts ? "T2" : "T1"; + final RecordMetaData withConflict = runRenameBatchedOnly(loadMetaData("TwoBoringTypes.json").build(), + oldName -> oldName.equals(other) ? prefix + conflicting : oldName, + // The undo must map each type back to the name it had originally. `runRename()` compares the union + // field number of each type with that of the original, so a wrong mapping will fail the comparison. + // The type now holding the prefixed name of the conflicting one used to be the other one. + newName -> newName.equals(prefix + conflicting) ? other : newName); assertEquals(Set.of(conflicting, prefix + conflicting), withConflict.getRecordTypes().keySet()); final RecordMetaData prefixed = runRenameBatchedOnly(withConflict.toProto(), @@ -1776,9 +1866,11 @@ private static RecordMetaData basicRenameAsserts(final RecordMetaDataProto.MetaD renamed.getRecordTypes().values().stream().map(RecordType::getName) .collect(Collectors.toSet())); for (final RecordType type : renamed.getRecordTypes().values()) { - assertEquals(type.getAllIndexes(), - originalMetaData.getRecordType(undoRename.apply(type.getName())) - .getAllIndexes()); + final RecordType originalType = originalMetaData.getRecordType(undoRename.apply(type.getName())); + assertEquals(type.getAllIndexes(), originalType.getAllIndexes()); + // Records of the type must still be written under the same union field. + assertEquals(originalMetaData.getUnionFieldForRecordType(originalType).getNumber(), + renamed.getUnionFieldForRecordType(type).getNumber()); } assertEquals(originalMetaData.getUniversalIndexes(), renamed.getUniversalIndexes()); return renamed;