Skip to content

Make MetaDataProtoEditor always rename the canonically named union field #4731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robert-brunel wants to merge 1 commit into from
Open

Make MetaDataProtoEditor always rename the canonically named union field #4731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ...rc/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,12 +971,14 @@
if (rename == null) {
continue;
}

Check notice on line 974 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 96.8% (484/500 lines) | Changed lines: 100.0% (2/2 lines)
// 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;
}
}
Expand Down
116 changes: 104 additions & 12 deletions ...java/com/apple/foundationdb/record/provider/foundationdb/MetaDataProtoEditorUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,24 @@ public static Stream<Arguments> 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()
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
Expand Down
Loading