From 98deb9df471f2e531d30298f7d2140e43a030029 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Tue, 28 Apr 2026 18:40:51 +0100 Subject: [PATCH 1/5] `MetaDataEvolutionValidator` now validates synthetic record type evolution The `MetaDataEvolutionValidator` has been udpated here to look at the synthetic types in the meta-data. This allows it to validate that the data held in join indexes and unnested indexes is not incorrectly updated during meta-data evolution. There are largely analogous paths followed for the two current synthetic types, `JoinedRecordType`s and `UnnestedRecordType`s. This resolves #4022. --- .../metadata/MetaDataEvolutionValidator.java | 255 +++++++++++++- .../MetaDataEvolutionValidatorTest.java | 313 ++++++++++++------ 2 files changed, 460 insertions(+), 108 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index d3e41581a0..d1d6d3ad36 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -40,6 +40,7 @@ import com.google.protobuf.Descriptors.FieldDescriptor; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -154,6 +155,7 @@ public void validate(@Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaDat validateUnion(oldMetaData.getUnionDescriptor(), newMetaData.getUnionDescriptor()); Map typeRenames = getTypeRenames(oldMetaData.getUnionDescriptor(), newMetaData.getUnionDescriptor()); validateRecordTypes(oldMetaData, newMetaData, typeRenames); + validateSyntheticTypes(oldMetaData, newMetaData, typeRenames); validateCurrentAndFormerIndexes(oldMetaData, newMetaData, typeRenames); } @@ -440,6 +442,255 @@ private void validateRecordTypes(@Nonnull RecordMetaData oldMetaData, @Nonnull R } } + private void validateSyntheticTypes(@Nonnull RecordMetaData oldMetaData, @Nonnull RecordMetaData newMetaData, + @Nonnull Map typeRenames) { + final Map> oldSyntheticTypes = getSyntheticTypesByRecordTypeKey(oldMetaData); + if (oldSyntheticTypes.isEmpty()) { + return; + } + final Map> newSyntheticTypes = getSyntheticTypesByRecordTypeKey(newMetaData); + for (Map.Entry> oldEntry : oldSyntheticTypes.entrySet()) { + Object recordTypeKey = oldEntry.getKey(); + final SyntheticRecordType oldSyntheticType = oldEntry.getValue(); + @Nullable SyntheticRecordType newSyntheticType = newSyntheticTypes.get(recordTypeKey); + if (newSyntheticType == null) { + // If the newSyntheticType is null, then it has been removed. This would mean that all the + // indexes that reference it have likewise been removed (or the meta-data validator would fail). + // So, we can ignore it + continue; + } + validateSyntheticType(oldSyntheticType, newSyntheticType, typeRenames); + } + } + + @Nonnull + private Map> getSyntheticTypesByRecordTypeKey(@Nonnull RecordMetaData metaData) { + final Map> byTypeKeyMap = new HashMap<>(); + for (SyntheticRecordType syntheticType : metaData.getSyntheticRecordTypes().values()) { + byTypeKeyMap.put(syntheticType.getRecordTypeKey(), syntheticType); + } + return byTypeKeyMap; + } + + private void validateSyntheticType(@Nonnull SyntheticRecordType oldType, @Nonnull SyntheticRecordType newType, @Nonnull Map typeRenames) { + if (disallowTypeRenames && !oldType.getName().equals(newType.getName())) { + throw new MetaDataException("synthetic record type name changed", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + } + if (!(oldType.getClass().equals(newType.getClass()))) { + throw new MetaDataException("synthetic record type changed type", + LogMessageKeys.EXPECTED, oldType.getName(), + LogMessageKeys.ACTUAL, newType.getClass().getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + if (oldType instanceof JoinedRecordType) { + validateJoinedRecordType((JoinedRecordType) oldType, (JoinedRecordType) newType, typeRenames); + } else if (oldType instanceof UnnestedRecordType) { + validateUnnestedRecordType((UnnestedRecordType) oldType, (UnnestedRecordType) newType, typeRenames); + } else { + throw new MetaDataException("unknown synthetic record type", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName(), + LogMessageKeys.RECORD_TYPE, oldType.getClass().getName()); + } + } + + private void validateJoinedRecordType(@Nonnull JoinedRecordType oldType, @Nonnull JoinedRecordType newType, @Nonnull Map typeRenames) { + // Validate constituents + final List oldConstituents = oldType.getConstituents(); + final List newConstituents = newType.getConstituents(); + if (oldConstituents.size() != newConstituents.size()) { + throw new MetaDataException("join constituent count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + for (int i = 0; i < oldConstituents.size(); i++) { + // We use the definition order of join constituents here to find a correspondence between + // old and new types. This is important because the position in the definition is how the + // logic within the code identifies different constituents (e.g., the primary key component + // at position i + 1 is the nested primary key of the i'th constituent). + JoinedRecordType.JoinConstituent oldConstituent = oldConstituents.get(i); + JoinedRecordType.JoinConstituent newConstituent = newConstituents.get(i); + + if (!allowFieldRenames && !oldConstituent.getName().equals(newConstituent.getName())) { + // The join constituent names are used in things like index field declarations, so changing + // them is analogous to a field change. Unlike regular field names, the name shouldn't appear + // in queries (only internal data structures like index definitions), so it may be okay to + // loosen this. + throw new MetaDataException("join constituent name changed", + LogMessageKeys.EXPECTED, oldConstituent.getName(), + LogMessageKeys.ACTUAL, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final String expectedTypeName = typeRenames.getOrDefault(oldConstituent.getRecordType().getName(), oldConstituent.getRecordType().getName()); + if (!expectedTypeName.equals(newConstituent.getRecordType().getName())) { + throw new MetaDataException("join constituent type changed", + LogMessageKeys.OLD_RECORD_TYPE, oldConstituent.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newConstituent.getName(), + LogMessageKeys.NAME, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + if (oldConstituent.isOuterJoined() != newConstituent.isOuterJoined()) { + throw new MetaDataException("join constituent outer-joined property changed", + LogMessageKeys.EXPECTED, oldConstituent.isOuterJoined(), + LogMessageKeys.ACTUAL, newConstituent.isOuterJoined(), + LogMessageKeys.NAME, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + + // Validate joins + final List oldJoins = oldType.getJoins(); + final List newJoins = newType.getJoins(); + if (oldJoins.size() != newJoins.size()) { + throw new MetaDataException("join type join count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getClass().getName()); + } + final Map oldNameToConstituentPosMap = constituentNameToPositionMap(oldConstituents); + final Map newNameToConstituentPosMap = constituentNameToPositionMap(newConstituents); + for (int i = 0; i < oldJoins.size(); i++) { + // Technically, these aren't ordered, but we'd otherwise need to enumerate the different joins by constituent + // pair and then search for ones that match. It is simpler here to just require that the joins be in the + // same order + JoinedRecordType.Join oldJoin = oldJoins.get(i); + JoinedRecordType.Join newJoin = newJoins.get(i); + + // Validate the left and right constituents are the same + int oldLeftPos = oldNameToConstituentPosMap.get(oldJoin.getLeft().getName()); + int newLeftPos = newNameToConstituentPosMap.get(newJoin.getLeft().getName()); + if (oldLeftPos != newLeftPos) { + throw new MetaDataException("join changed left constituent", + LogMessageKeys.EXPECTED, newConstituents.get(oldLeftPos).getName(), + LogMessageKeys.ACTUAL, newConstituents.get(newLeftPos).getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final KeyExpression expectedLeftExpression; + if (allowsAnyFieldRenames()) { + Descriptors.Descriptor oldDescriptor = oldConstituents.get(oldLeftPos).getRecordType().getDescriptor(); + Descriptors.Descriptor newDescriptor = newConstituents.get(newLeftPos).getRecordType().getDescriptor(); + expectedLeftExpression = RenameFieldsVisitor.renameFields(oldJoin.getLeftExpression(), oldDescriptor, newDescriptor); + } else { + expectedLeftExpression = oldJoin.getLeftExpression(); + } + if (!expectedLeftExpression.equals(newJoin.getLeftExpression())) { + throw new MetaDataException("join changed left expression", + LogMessageKeys.EXPECTED, expectedLeftExpression, + LogMessageKeys.ACTUAL, newJoin.getLeftExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + int oldRightPos = oldNameToConstituentPosMap.get(oldJoin.getRight().getName()); + int newRightPos = newNameToConstituentPosMap.get(newJoin.getRight().getName()); + if (oldRightPos != newRightPos) { + throw new MetaDataException("join changed right constituent", + LogMessageKeys.EXPECTED, newConstituents.get(oldRightPos).getName(), + LogMessageKeys.ACTUAL, newConstituents.get(newRightPos).getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + final KeyExpression expectedRightExpression; + if (allowsAnyFieldRenames()) { + Descriptors.Descriptor oldDescriptor = oldConstituents.get(oldRightPos).getRecordType().getDescriptor(); + Descriptors.Descriptor newDescriptor = newConstituents.get(newRightPos).getRecordType().getDescriptor(); + expectedRightExpression = RenameFieldsVisitor.renameFields(oldJoin.getRightExpression(), oldDescriptor, newDescriptor); + } else { + expectedRightExpression = oldJoin.getLeftExpression(); + } + if (!expectedRightExpression.equals(newJoin.getRightExpression())) { + throw new MetaDataException("join changed right expression", + LogMessageKeys.EXPECTED, expectedRightExpression, + LogMessageKeys.ACTUAL, newJoin.getRightExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + } + + private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @Nonnull UnnestedRecordType newType, @Nonnull Map typeRenames) { + // Validate constituents + final List oldConstituents = oldType.getConstituents(); + final List newConstituents = newType.getConstituents(); + if (oldConstituents.size() != newConstituents.size()) { + throw new MetaDataException("unnested type constituent count changed", + LogMessageKeys.EXPECTED, oldConstituents.size(), + LogMessageKeys.ACTUAL, newConstituents.size(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + // We will need this to validate that the parent constituents refer to the same entity, even in the presence of renames + final Map oldNameToConstituentPosMap = constituentNameToPositionMap(oldConstituents); + final Map newNameToConstituentPosMap = constituentNameToPositionMap(newConstituents); + + for (int i = 0; i < oldConstituents.size(); i++) { + UnnestedRecordType.NestedConstituent oldConstituent = oldConstituents.get(i); + UnnestedRecordType.NestedConstituent newConstituent = newConstituents.get(i); + + if (!allowFieldRenames && !oldConstituent.getName().equals(newConstituent.getName())) { + throw new MetaDataException("nested constituent name changed", + LogMessageKeys.EXPECTED, oldConstituent.getName(), + LogMessageKeys.ACTUAL, newConstituent.getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + if (oldConstituent.isParent() != newConstituent.isParent()) { + throw new MetaDataException("nested constituent parent property changed", + LogMessageKeys.EXPECTED, oldConstituent.isParent(), + LogMessageKeys.ACTUAL, newConstituent.isParent(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + if (oldConstituent.isParent()) { + // Make sure the overall parent has the same record type (subject to renames) + final String oldRecordType = oldConstituent.getRecordType().getName(); + final String expectedTypeName = typeRenames.getOrDefault(oldRecordType, oldRecordType); + if (!expectedTypeName.equals(newConstituent.getRecordType().getName())) { + throw new MetaDataException("unnested type parent record type changed", + LogMessageKeys.EXPECTED, expectedTypeName, + LogMessageKeys.ACTUAL, newConstituent.getRecordType().getName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } else { + // Make sure we are pointing to the same parent + int oldParentPos = oldNameToConstituentPosMap.get(oldConstituent.getName()); + int newParentPos = newNameToConstituentPosMap.get(newConstituent.getName()); + if (oldParentPos != newParentPos) { + throw new MetaDataException("nested constituent changed parent", + LogMessageKeys.EXPECTED, newConstituents.get(oldParentPos).getName(), + LogMessageKeys.ACTUAL, newConstituent.getParentName(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + + // Make sure the nesting expression is still the same, possibly accounting for field renames + final KeyExpression expectedNestingExpression; + if (allowsAnyFieldRenames()) { + UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); + UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); + expectedNestingExpression = RenameFieldsVisitor.renameFields(oldConstituent.getNestingExpression(), oldParent.getRecordType().getDescriptor(), newParent.getRecordType().getDescriptor()); + } else { + expectedNestingExpression = oldConstituent.getNestingExpression(); + } + if (!expectedNestingExpression.equals(newConstituent.getNestingExpression())) { + throw new MetaDataException("nested constituent nesting expression changed", + LogMessageKeys.EXPECTED, expectedNestingExpression, + LogMessageKeys.ACTUAL, newConstituent.getNestingExpression(), + LogMessageKeys.RECORD_TYPE, newType.getName()); + } + } + } + } + + @Nonnull + private Map constituentNameToPositionMap(@Nonnull List constituents) { + final Map nameToPositionMap = new HashMap<>(); + for (int i = 0; i < constituents.size(); i++) { + nameToPositionMap.put(constituents.get(i).getName(), i); + } + return nameToPositionMap; + } + @Nonnull private Map getFormerIndexMap(@Nonnull RecordMetaData metaData) { Map formerIndexMap; @@ -690,8 +941,8 @@ private void validateIndex(@Nonnull RecordMetaData oldMetaData, @Nonnull Index o KeyExpression expectedKeyExpression = null; if (allowsAnyFieldRenames()) { for (String oldRecordTypeName : oldRecordTypeNames) { - final Descriptor oldDescriptor = oldMetaData.getRecordType(oldRecordTypeName).getDescriptor(); - final Descriptor newDescriptor = newMetaData.getRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor(); + final Descriptor oldDescriptor = oldMetaData.getIndexableRecordType(oldRecordTypeName).getDescriptor(); + final Descriptor newDescriptor = newMetaData.getIndexableRecordType(typeRenames.getOrDefault(oldRecordTypeName, oldRecordTypeName)).getDescriptor(); final KeyExpression renamedKeyExpression = RenameFieldsVisitor.renameFields(oldIndex.getRootExpression(), oldDescriptor, newDescriptor); if (expectedKeyExpression == null) { expectedKeyExpression = renamedKeyExpression; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java index 56f68af1d3..a383522313 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java @@ -66,7 +66,6 @@ import java.util.Locale; import java.util.Set; import java.util.function.Consumer; -import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -1030,8 +1029,8 @@ void renameFieldInUniversalIndex(boolean deprecated) { // Still not allowed as the index hasn't been updated fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData3, "all$num_value_2", - indexProto -> indexProto.toBuilder().setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()).build()); + RecordMetaData metaData4 = mutateIndex(metaData3, "all$num_value_2", + indexProto -> indexProto.setRootExpression(Key.Expressions.field("num_value_2__old").toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData4); } @@ -1370,8 +1369,8 @@ void nestedTypesMergedWithIndexesAndFieldRenames(boolean deprecated) { fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.a_prime -> a.b - final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b"), Key.Expressions.field("b").nest("b")).toKeyExpression()).build()); + final RecordMetaData metaData3 = mutateIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> + indexProto.setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b"), Key.Expressions.field("b").nest("b")).toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } @@ -1446,8 +1445,8 @@ void nestedTypesSplitWithIndex(boolean deprecated) { fieldRenameChecker.assertInvalidRenaming("index key expression does not match required", deprecated, metaData1, metaData2); // Update the index so that it reflects the new field name for a.b -> a.b_1 and b.b -> b.b_2 - final RecordMetaData metaData3 = replaceIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b_1"), Key.Expressions.field("b").nest("b_2")).toKeyExpression()).build()); + final RecordMetaData metaData3 = mutateIndex(metaData2, "MyRecord$a.b+b.b", indexProto -> + indexProto.setRootExpression(Key.Expressions.concat(Key.Expressions.field("a").nest("b_1"), Key.Expressions.field("b").nest("b_2")).toKeyExpression())); fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } @@ -1635,6 +1634,102 @@ void primaryKeyChanged() { assertInvalid("record type primary key changed", metaData1, metaData2); } + // Synthetic type tests + + @Test + void addJoinedType() { + final RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); + + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void dropJoinedType() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + final RecordMetaData metaData1 = metaDataBuilder.build(); + + metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + metaDataBuilder.addFormerIndex(new FormerIndex("joined$l.num_value_unique", metaData1.getVersion(), metaData1.getVersion() + 1, "joined$l.num_value_unique")); + RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void swapJoinConstituents() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + + final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedType -> { + final var constituentsList = joinedType.getJoinConstituentsList(); + joinedType.clearJoinConstituents(); + joinedType.addJoinConstituents(constituentsList.get(1)); + joinedType.addJoinConstituents(constituentsList.get(0)); + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("join constituent type changed", laxerValidator, metaData1, metaData2); + } + + @Test + void swapSelfJoinConstituents() { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); + joinBuilder.addConstituent("s1", "MySimpleRecord"); + joinBuilder.addConstituent("s2", "MySimpleRecord"); + joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); + metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + + final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "self_join_svi", joinedType -> { + final var constituentsList = joinedType.getJoinConstituentsList(); + joinedType.clearJoinConstituents(); + joinedType.addJoinConstituents(constituentsList.get(1)); + joinedType.addJoinConstituents(constituentsList.get(0)); + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("join changed left constituent", laxerValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "self_join_svi", + // Swap the names in the Join definition + joinedType -> joinedType.getJoinsBuilder(0).setLeft("s2").setRight("s1")); + assertInvalid("join constituent name changed", metaData1, metaData3); + assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData3); + + final RecordMetaData metaData4 = mutateIndex(metaData3, "self_join_svi$num_value_unique", + // Swap the names in the index definition + index -> index.setRootExpression(Key.Expressions.concat(Key.Expressions.field("s2").nest("num_value_unique"), Key.Expressions.field("s1").nest("num_value_unique")).toKeyExpression())); + assertInvalid("join constituent name changed", metaData1, metaData4); + laxerValidator.validate(metaData1, metaData4); + } + // Former index tests @Test @@ -1839,12 +1934,11 @@ void removeIndexAndChangeLastModifiedVersion() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); // Step 1: Update the index definition in a way that updates the last modified version - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", index -> - // Mark the index as unique (and bump its last modified version - index.toBuilder() - .addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey("unique").setValue("true")) - .setLastModifiedVersion(index.getLastModifiedVersion() + 1) - .build()); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", index -> { + // Mark the index as unique (and bump its last modified version + makeUnique(index); + index.setLastModifiedVersion(index.getLastModifiedVersion() + 1); + }); assertFalse(validator.allowsIndexRebuilds()); assertInvalid("last modified version of index changed", metaData1, metaData2); @@ -1892,52 +1986,65 @@ void defaultIndexRemovalPath() { // Index tests - @Nonnull - private RecordMetaDataProto.Index changeOption(@Nonnull RecordMetaDataProto.Index indexProto, @Nonnull String key, @Nullable String value) { - RecordMetaDataProto.Index.Builder builder = indexProto.toBuilder(); + private void changeOption(@Nonnull RecordMetaDataProto.Index.Builder indexProto, @Nonnull String key, @Nullable String value) { boolean found = false; - for (int i = 0; i < builder.getOptionsCount(); i++) { - final RecordMetaDataProto.Index.Option option = builder.getOptions(i); + int i = 0; + for (RecordMetaDataProto.Index.Option.Builder option : indexProto.getOptionsBuilderList()) { if (key.equals(option.getKey())) { - if (value == null) { - builder.removeOptions(i); - } else { - builder.setOptions(i, RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); + if (value != null) { + option.setValue(value); } found = true; break; } + i++; } - if (!found && value != null) { - builder.addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); + if (found && value == null) { + indexProto.removeOptions(i); + } else if (!found && value != null) { + indexProto.addOptions(RecordMetaDataProto.Index.Option.newBuilder().setKey(key).setValue(value)); } - return builder.build(); } - @Nonnull - private RecordMetaDataProto.Index makeUnique(@Nonnull RecordMetaDataProto.Index indexProto) { - return changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "true"); + private void makeUnique(@Nonnull RecordMetaDataProto.Index.Builder indexProto) { + changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "true"); } @Nonnull - private RecordMetaDataProto.Index clearOptions(@Nonnull RecordMetaDataProto.Index indexProto) { - return indexProto.toBuilder().clearOptions().build(); + private void clearOptions(@Nonnull RecordMetaDataProto.Index.Builder indexProto) { + indexProto.clearOptions(); } @Nonnull - private RecordMetaData replaceIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, UnaryOperator indexReplacement) { - RecordMetaDataProto.MetaData metaDataProto = metaData.toProto(); - RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaDataProto.toBuilder(); + private RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { + RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); + updater.accept(metaDataProtoBuilder); metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); - for (int i = 0; i < metaDataProto.getIndexesCount(); i ++) { - RecordMetaDataProto.Index indexProto = metaDataProto.getIndexes(i); - if (indexProto.getName().equals(indexName)) { - metaDataProtoBuilder.setIndexes(i, indexReplacement.apply(indexProto)); - } - } return RecordMetaData.build(metaDataProtoBuilder.build()); } + @Nonnull + private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, @Nonnull Consumer indexMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.Index.Builder indexProto : metaDataProtoBuilder.getIndexesBuilderList()) { + if (indexProto.getName().equals(indexName)) { + indexMutator.accept(indexProto); + } + } + }); + } + + @Nonnull + private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } + @Test void silentlyRemoveIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); @@ -1972,8 +2079,8 @@ void indexSubspaceKeyChanged() { // The index subspace key is the thing that determines whether an index is even there, so changing it // is identical to removing the index RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setSubspaceKey(ByteString.copyFrom(Tuple.from("dummy_key").pack())).build() + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> + indexProto.setSubspaceKey(ByteString.copyFrom(Tuple.from("dummy_key").pack())) ); assertInvalid("index missing in new meta-data", metaData1, metaData2); } @@ -1981,11 +2088,10 @@ void indexSubspaceKeyChanged() { @Test void indexNameChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder() + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> + indexProto .setSubspaceKey(ByteString.copyFrom(Tuple.from("MySimpleRecord$str_value_indexed").pack())) .setName("a_different_name") - .build() ); assertInvalid("index name changed", metaData1, metaData2); } @@ -1993,34 +2099,29 @@ void indexNameChanged() { @Test void indexAddedVersionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setAddedVersion(metaData1.getVersion() + 1).setLastModifiedVersion(metaData1.getVersion() + 1).build() - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setAddedVersion(metaData1.getVersion() + 1).setLastModifiedVersion(metaData1.getVersion() + 1)); assertInvalid("new index added version does not match old index added version", metaData1, metaData2); - metaData2 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setAddedVersion(indexProto.getAddedVersion() - 1).build() - ); + metaData2 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setAddedVersion(indexProto.getAddedVersion() - 1)); assertInvalid("new index added version does not match old index added version", metaData1, metaData2); } @Test void indexLastModifiedVersionTooOld() { - RecordMetaData metaData1 = replaceIndex(RecordMetaData.build(TestRecords1Proto.getDescriptor()), "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(2).build() - ); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(1).build() - ); + RecordMetaData metaData1 = mutateIndex(RecordMetaData.build(TestRecords1Proto.getDescriptor()), "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(2)); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(1)); assertInvalid("old index has last-modified version newer than new index", metaData1, metaData2); } @Test void indexLastModifiedVersionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setLastModifiedVersion(metaData1.getVersion() + 1).build() - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setLastModifiedVersion(metaData1.getVersion() + 1)); assertFalse(validator.allowsIndexRebuilds()); assertInvalid("last modified version of index changed", metaData1, metaData2); @@ -2031,18 +2132,17 @@ void indexLastModifiedVersionChanged() { laxerValidator.validate(metaData1, metaData2); } - private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaData metaData1, @Nonnull String indexName, UnaryOperator indexReplacement) { + private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaData metaData1, @Nonnull String indexName, Consumer indexReplacement) { MetaDataEvolutionValidator laxerValidator = MetaDataEvolutionValidator.newBuilder() .setAllowIndexRebuilds(true) .build(); - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, indexReplacement); + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexReplacement); assertInvalid(errMsg, metaData1, metaData2); assertInvalid(errMsg, laxerValidator, metaData1, metaData2); // Allow the change if and only if the last modified version is updated and the option allowing rebuilds is set - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, indexProto -> - indexProto.toBuilder().setLastModifiedVersion(metaData2.getVersion()).build() - ); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, + indexProto -> indexProto.setLastModifiedVersion(metaData2.getVersion())); assertInvalid("last modified version of index changed", metaData1, metaData3); laxerValidator.validate(metaData1, metaData3); } @@ -2050,17 +2150,16 @@ private void validateIndexMutation(@Nonnull String errMsg, @Nonnull RecordMetaDa @Test void indexTypeChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - validateIndexMutation("index type changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setType(IndexTypes.RANK).build() + validateIndexMutation("index type changed", metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setType(IndexTypes.RANK) ); } @Test void indexKeyExpressionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - validateIndexMutation("index key expression changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - indexProto.toBuilder().setRootExpression(Key.Expressions.field("num_value_2").toKeyExpression()).build() - ); + validateIndexMutation("index key expression changed", metaData1, "MySimpleRecord$str_value_indexed", + indexProto -> indexProto.setRootExpression(Key.Expressions.field("num_value_2").toKeyExpression())); } @Test @@ -2070,8 +2169,8 @@ void indexRecordTypeRemoved() { metaDataBuilder.addMultiTypeIndex(Arrays.asList(metaDataBuilder.getRecordType("MySimpleRecord"), metaDataBuilder.getRecordType("MyOtherRecord")), new Index(indexName, "num_value_2")); RecordMetaData metaData1 = metaDataBuilder.getRecordMetaData(); - validateIndexMutation("new index removes record type", metaData1, indexName, indexProto -> - indexProto.toBuilder().clearRecordType().addRecordType("MySimpleRecord").build() + validateIndexMutation("new index removes record type", metaData1, indexName, + indexProto -> indexProto.clearRecordType().addRecordType("MySimpleRecord") ); } @@ -2079,14 +2178,14 @@ void indexRecordTypeRemoved() { void indexRecordTypeAdded() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); validateIndexMutation("new index adds record type that is not newer than old meta-data", metaData1, "MySimpleRecord$num_value_3_indexed", indexProto -> - indexProto.toBuilder().addRecordType("MyOtherRecord").build() + indexProto.addRecordType("MyOtherRecord").build() ); // Add NewRecord as a record type to the existing meta-data and index and validate that this change is okay // because the new record type is newer the old meta-data version. RecordMetaData tempMetaData = addNewRecordType(metaData1); - RecordMetaData metaData2 = replaceIndex(tempMetaData, "MySimpleRecord$num_value_3_indexed", indexProto -> - indexProto.toBuilder().addRecordType("NewRecord").build()); + RecordMetaData metaData2 = mutateIndex(tempMetaData, "MySimpleRecord$num_value_3_indexed", + indexProto -> indexProto.addRecordType("NewRecord")); validator.validate(metaData1, metaData2); // valid if type and index change happen together assertInvalid("new index adds record type that is not newer than old meta-data", tempMetaData, metaData2); } @@ -2099,8 +2198,8 @@ void indexPrimaryKeyComponentsChanged() { assertThat(metaData1.getIndex("rec_no").hasPrimaryKeyComponentPositions(), is(true)); RecordMetaData tempMetaData = addNewRecordType(metaData1); - RecordMetaData metaData2 = replaceIndex(tempMetaData, "rec_no", indexProto -> - indexProto.toBuilder().addRecordType("NewRecord").build()); + RecordMetaData metaData2 = mutateIndex(tempMetaData, "rec_no", + indexProto -> indexProto.addRecordType("NewRecord")); assertInvalid("new index drops primary key component positions", metaData1, metaData2); // This is essentially the behavior change outlined by: https://github.com/FoundationDB/fdb-record-layer/issues/93 @@ -2128,8 +2227,8 @@ void addRecordTypeWithUniversalIndex() { validator.validate(metaData1, metaData2); // Make the index a multi-type index on the original record types - RecordMetaData metaData3 = replaceIndex(metaData2, "rec_no", indexProto -> - indexProto.toBuilder().addRecordType("MySimpleRecord").addRecordType("MyOtherRecord").build()); + RecordMetaData metaData3 = mutateIndex(metaData2, "rec_no", + indexProto -> indexProto.addRecordType("MySimpleRecord").addRecordType("MyOtherRecord")); MetaDataException e = assertThrows(MetaDataException.class, () -> metaData3.getUniversalIndex("rec_no")); assertThat(e.getMessage(), containsString("Index rec_no not defined")); assertInvalid("new index removes record type", metaData2, metaData3); @@ -2143,10 +2242,10 @@ void uniquenessConstraintChanged() { validateIndexMutation("index adds uniqueness constraint", metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); // Removing the uniqueness constraint is fine - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); validator.validate(metaData2, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "false")); + RecordMetaData metaData4 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, "false")); validator.validate(metaData2, metaData4); } @@ -2154,25 +2253,25 @@ void uniquenessConstraintChanged() { void allowedForQueriesChanged() { // Changing this option is always fine RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.ALLOWED_FOR_QUERY_OPTION, "false")); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.ALLOWED_FOR_QUERY_OPTION, "true")); validator.validate(metaData2, metaData3); - RecordMetaData metaData4 = replaceIndex(metaData3, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData4 = mutateIndex(metaData3, "MySimpleRecord$str_value_indexed", this::clearOptions); validator.validate(metaData3, metaData4); } @Test void changeReplacedByIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX, "MySimpleRecord$num_value_3_indexed")); assertEquals(Collections.singletonList("MySimpleRecord$num_value_3_indexed"), metaData2.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); assertEquals(Collections.emptyList(), metaData3.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData2, metaData3); @@ -2181,14 +2280,14 @@ void changeReplacedByIndex() { @Test void changeReplacedByIndexSet() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> - changeOption(changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_0", "MySimpleRecord$num_value_3_indexed"), - IndexOptions.REPLACED_BY_OPTION_PREFIX + "_1", "MySimpleRecord$num_value_unique") - ); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> { + changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_0", "MySimpleRecord$num_value_3_indexed"); + changeOption(indexProto, IndexOptions.REPLACED_BY_OPTION_PREFIX + "_1", "MySimpleRecord$num_value_unique"); + }); assertThat(metaData2.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames(), containsInAnyOrder("MySimpleRecord$num_value_3_indexed", "MySimpleRecord$num_value_unique")); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", this::clearOptions); assertEquals(Collections.emptyList(), metaData3.getIndex("MySimpleRecord$str_value_indexed").getReplacedByIndexNames()); validator.validate(metaData2, metaData3); @@ -2199,9 +2298,11 @@ void unknownOptionChanged() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); validateIndexMutation("index option changed", metaData1, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, "dummyOption", "dummyValue")); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", - indexProto -> makeUnique(changeOption(indexProto, "dummyOption", "dummyValue"))); - RecordMetaData metaData3 = replaceIndex(metaData2, "MySimpleRecord$str_value_indexed", + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", indexProto -> { + makeUnique(indexProto); + changeOption(indexProto, "dummyOption", "dummyValue"); + }); + RecordMetaData metaData3 = mutateIndex(metaData2, "MySimpleRecord$str_value_indexed", indexProto -> changeOption(indexProto, IndexOptions.UNIQUE_OPTION, null)); validator.validate(metaData2, metaData3); validateIndexMutation("index option changed", metaData3, "MySimpleRecord$str_value_indexed", @@ -2221,10 +2322,10 @@ void rankLevelsChanged() { indexProto -> changeOption(indexProto, IndexOptions.RANK_NLEVELS, "" + RankedSet.MAX_LEVELS)); // Setting the default explicitly is fine - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexProto -> changeOption(indexProto, IndexOptions.RANK_NLEVELS, "" + RankedSet.DEFAULT_LEVELS)); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, this::clearOptions); validator.validate(metaData2, metaData3); } @@ -2239,37 +2340,37 @@ void textOptionsChanged() { indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, AllSuffixesTextTokenizer.NAME)); // Setting the default explicitly is fine - RecordMetaData metaData2 = replaceIndex(metaData1, indexName, + RecordMetaData metaData2 = mutateIndex(metaData1, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, DefaultTextTokenizer.NAME)); validator.validate(metaData1, metaData2); - RecordMetaData metaData3 = replaceIndex(metaData2, indexName, this::clearOptions); + RecordMetaData metaData3 = mutateIndex(metaData2, indexName, this::clearOptions); validator.validate(metaData2, metaData3); // Increasing the tokenizer version is fine, but decreasing it is not - RecordMetaData metaData4 = replaceIndex(metaData3, indexName, + RecordMetaData metaData4 = mutateIndex(metaData3, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_NAME_OPTION, PrefixTextTokenizer.NAME)); - RecordMetaData metaData5 = replaceIndex(metaData4, indexName, + RecordMetaData metaData5 = mutateIndex(metaData4, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + TextTokenizer.GLOBAL_MIN_VERSION)); validator.validate(metaData4, metaData5); - RecordMetaData metaData6 = replaceIndex(metaData5, indexName, + RecordMetaData metaData6 = mutateIndex(metaData5, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + (TextTokenizer.GLOBAL_MIN_VERSION + 1))); validator.validate(metaData5, metaData6); validateIndexMutation("text tokenizer version downgraded", metaData6, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_TOKENIZER_VERSION_OPTION, "" + TextTokenizer.GLOBAL_MIN_VERSION)); // Changing whether aggressive conflict ranges are allowed is safe - RecordMetaData metaData7 = replaceIndex(metaData6, indexName, + RecordMetaData metaData7 = mutateIndex(metaData6, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_ADD_AGGRESSIVE_CONFLICT_RANGES_OPTION, "true")); validator.validate(metaData6, metaData7); - RecordMetaData metaData8 = replaceIndex(metaData7, indexName, + RecordMetaData metaData8 = mutateIndex(metaData7, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_ADD_AGGRESSIVE_CONFLICT_RANGES_OPTION, "false")); validator.validate(metaData7, metaData8); // Changing whether position lists are omitted is safe - RecordMetaData metaData9 = replaceIndex(metaData8, indexName, + RecordMetaData metaData9 = mutateIndex(metaData8, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_OMIT_POSITIONS_OPTION, "true")); validator.validate(metaData8, metaData9); - RecordMetaData metaData10 = replaceIndex(metaData9, indexName, + RecordMetaData metaData10 = mutateIndex(metaData9, indexName, indexProto -> changeOption(indexProto, IndexOptions.TEXT_OMIT_POSITIONS_OPTION, "false")); validator.validate(metaData9, metaData10); } @@ -2277,7 +2378,7 @@ void textOptionsChanged() { @Test void optionChangeAllowedWithCustomIndexValidatorRegistry() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); - RecordMetaData metaData2 = replaceIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); + RecordMetaData metaData2 = mutateIndex(metaData1, "MySimpleRecord$str_value_indexed", this::makeUnique); assertSame(IndexMaintainerFactoryRegistryImpl.instance(), validator.getIndexValidatorRegistry()); assertInvalid("index adds uniqueness constraint", metaData1, metaData2); From 6d4553aecb91f4544e2377846536b02b5944a403 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 15:18:48 +0100 Subject: [PATCH 2/5] moar joined record type tests --- .../MetaDataEvolutionValidatorTest.java | 461 ++++++++++++++++-- 1 file changed, 408 insertions(+), 53 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java index a383522313..c54a50fd22 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java @@ -26,6 +26,7 @@ import com.apple.foundationdb.record.RecordMetaDataBuilder; import com.apple.foundationdb.record.RecordMetaDataOptionsProto; import com.apple.foundationdb.record.RecordMetaDataProto; +import com.apple.foundationdb.record.TestRecords1EvolvedProto; import com.apple.foundationdb.record.TestRecords1Proto; import com.apple.foundationdb.record.TestRecords4Proto; import com.apple.foundationdb.record.TestRecordsEnumProto; @@ -43,6 +44,7 @@ import com.apple.foundationdb.record.provider.common.text.DefaultTextTokenizer; import com.apple.foundationdb.record.provider.common.text.PrefixTextTokenizer; import com.apple.foundationdb.record.provider.common.text.TextTokenizer; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase; import com.apple.foundationdb.record.provider.foundationdb.IndexMaintainerFactoryRegistryImpl; import com.apple.foundationdb.tuple.Tuple; import com.apple.test.ParameterizedTestUtils; @@ -56,6 +58,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -238,15 +241,21 @@ void doNotChangeVersion() { // Schema evolution tests + @Nonnull + static RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { + RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); + metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); + updater.accept(metaDataProtoBuilder); + return RecordMetaData.build(metaDataProtoBuilder.build()); + } + @Nonnull static RecordMetaData replaceRecordsDescriptor(@Nonnull RecordMetaData metaData, @Nonnull FileDescriptor newDescriptor, @Nonnull Consumer metaDataMutation) { - RecordMetaDataProto.MetaData.Builder protoBuilder = metaData.toProto().toBuilder() - .setVersion(metaData.getVersion() + 1) - .setRecords(newDescriptor.toProto()) - .addDependencies(TestRecords1Proto.getDescriptor().toProto()); - metaDataMutation.accept(protoBuilder); - return RecordMetaData.build(protoBuilder.build()); + return updateMetaData(metaData, protoBuilder -> { + protoBuilder.setRecords(newDescriptor.toProto()); + metaDataMutation.accept(protoBuilder); + }); } @Nonnull @@ -1634,7 +1643,65 @@ void primaryKeyChanged() { assertInvalid("record type primary key changed", metaData1, metaData2); } - // Synthetic type tests + // JoinedRecordType type tests + + private void addNumValue2Join(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("l", "MySimpleRecord"); + joinBuilder.addConstituent("r", "MyOtherRecord"); + joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); + + metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + } + + private void addThreeWayNumValue2Join(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); + joinBuilder.addConstituent("s", "MySimpleRecord"); + joinBuilder.addConstituent("o", "MyOtherRecord"); + joinBuilder.addConstituent("a", "AnotherRecord"); + joinBuilder.addJoin("s", Key.Expressions.field("num_value_2"), "o", Key.Expressions.field("num_value_2")); + joinBuilder.addJoin("s", Key.Expressions.field("num_value_2"), "a", Key.Expressions.field("num_value_2")); + + metaDataBuilder.addIndex("join_nv2", "joined$nv3", Key.Expressions.concat( + Key.Expressions.field("s").nest("num_value_3_indexed"), + Key.Expressions.field("o").nest("num_value_3_indexed"), + Key.Expressions.field("a").nest("num_value_3_indexed")) + ); + } + + private void addStrValueSelfJoin(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); + joinBuilder.addConstituent("s1", "MySimpleRecord"); + joinBuilder.addConstituent("s2", "MySimpleRecord"); + joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); + + metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + } + + @Nonnull + private RecordMetaData createSimpleMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Nonnull + private RecordMetaData createEvolvedMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1EvolvedProto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Nonnull + private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } @Test void addJoinedType() { @@ -1642,11 +1709,7 @@ void addJoinedType() { RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); metaDataBuilder.setVersion(metaData1.getVersion() + 1); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + addNumValue2Join(metaDataBuilder); RecordMetaData metaData2 = metaDataBuilder.build(); validator.validate(metaData1, metaData2); @@ -1654,15 +1717,9 @@ void addJoinedType() { @Test void dropJoinedType() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); - final RecordMetaData metaData1 = metaDataBuilder.build(); + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); - metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); metaDataBuilder.setVersion(metaData1.getVersion() + 1); metaDataBuilder.addFormerIndex(new FormerIndex("joined$l.num_value_unique", metaData1.getVersion(), metaData1.getVersion() + 1, "joined$l.num_value_unique")); RecordMetaData metaData2 = metaDataBuilder.build(); @@ -1670,16 +1727,37 @@ void dropJoinedType() { validator.validate(metaData1, metaData2); } + @Test + void swapJoinTypeDefinitions() { + final RecordMetaData metaData1 = createSimpleMetaData(metaDataBuilder -> { + addNumValue2Join(metaDataBuilder); + addStrValueSelfJoin(metaDataBuilder); + }); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Builder type1 = metaDataBuilder.getJoinedRecordTypesBuilder(0); + final RecordMetaDataProto.JoinedRecordType.Builder type2 = metaDataBuilder.getJoinedRecordTypesBuilder(1); + + // Swap the two types' record type keys + RecordKeyExpressionProto.Value type1Key = type1.getRecordTypeKey(); + type1.setRecordTypeKey(type2.getRecordTypeKey()); + type2.setRecordTypeKey(type1Key); + }); + + assertNotEquals(metaData1.getSyntheticRecordType("join_nv2").getRecordTypeKey(), metaData2.getSyntheticRecordType("join_nv2").getRecordTypeKey()); + assertNotEquals(metaData1.getSyntheticRecordType("self_join_svi").getRecordTypeKey(), metaData2.getSyntheticRecordType("self_join_svi").getRecordTypeKey()); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + } + @Test void swapJoinConstituents() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("join_nv2"); - joinBuilder.addConstituent("l", "MySimpleRecord"); - joinBuilder.addConstituent("r", "MyOtherRecord"); - joinBuilder.addJoin("l", Key.Expressions.field("num_value_2"), "r", Key.Expressions.field("num_value_2")); - metaDataBuilder.addIndex("join_nv2", "joined$l.num_value_unique", Key.Expressions.field("l").nest("num_value_unique")); + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); - final RecordMetaData metaData1 = metaDataBuilder.build(); final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedType -> { final var constituentsList = joinedType.getJoinConstituentsList(); joinedType.clearJoinConstituents(); @@ -1696,14 +1774,8 @@ void swapJoinConstituents() { @Test void swapSelfJoinConstituents() { - RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor()); - final JoinedRecordTypeBuilder joinBuilder = metaDataBuilder.addJoinedRecordType("self_join_svi"); - joinBuilder.addConstituent("s1", "MySimpleRecord"); - joinBuilder.addConstituent("s2", "MySimpleRecord"); - joinBuilder.addJoin("s1", Key.Expressions.field("str_value_indexed"), "s2", Key.Expressions.field("str_value_indexed")); - metaDataBuilder.addIndex("self_join_svi", "self_join_svi$num_value_unique", Key.Expressions.concat(Key.Expressions.field("s1").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique"))); + final RecordMetaData metaData1 = createSimpleMetaData(this::addStrValueSelfJoin); - final RecordMetaData metaData1 = metaDataBuilder.build(); final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "self_join_svi", joinedType -> { final var constituentsList = joinedType.getJoinConstituentsList(); joinedType.clearJoinConstituents(); @@ -1730,6 +1802,308 @@ void swapSelfJoinConstituents() { laxerValidator.validate(metaData1, metaData4); } + @ParameterizedTest + @ValueSource(strings = {"MySimpleRecord", "MyOtherRecord"}) + void changeUnderlyingJoinConstituentType(String recordType) { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Delete and recreate the MySimpleRecord type. As we can't actually delete types, this is done by + // renaming the old type and deprecating its field in the union descriptor. We then add a new copy of + // it to the list of types, ensuring to assign that new type a new union descriptor field. + // We use a recreated version of the same type to maximize the number of things that are the same: + // the same type name, the same set of fields, etc. The only way that it should notice that something + // is up is that it has to check the position in the union descriptor + FileDescriptor updatedDescriptor = mutateFile(TestRecords1Proto.getDescriptor(), fileBuilder -> { + DescriptorProtos.DescriptorProto oldType = null; + for (DescriptorProtos.DescriptorProto.Builder messageType : fileBuilder.getMessageTypeBuilderList()) { + if (messageType.getName().equals(recordType)) { + // Rename the old MySimpleRecord. Save a copy prior to the rename + oldType = messageType.build(); + messageType.setName(recordType + "__old"); + } else if (messageType.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + // Rename the reference in the union descriptor + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageType.getFieldBuilderList()) { + if (fieldBuilder.getTypeName().endsWith(recordType)) { + deprecateField(fieldBuilder); + fieldBuilder + .setName("_" + recordType + "__old") + .setTypeName(recordType + "__old"); + } + } + // Create a field for the recreated MySimpleRecord (added below) + addField(messageType) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName(recordType) + .setName("_" + recordType); + + } + } + // Add a copy of MySimpleRecord to the fileBuilder. This represents recreating a new type with + // the same name (and in this case, the same set of fields). + fileBuilder.addMessageType(oldType); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedDescriptor, metaDataBuilder -> { + // Rename the old record type + for (RecordMetaDataProto.RecordType.Builder recordTypeBuilder : metaDataBuilder.getRecordTypesBuilderList()) { + if (recordTypeBuilder.getName().equals(recordType)) { + recordTypeBuilder.setName(recordType + "__old"); + } + } + // Create a new record type with an updated since version + metaDataBuilder.addRecordTypesBuilder() + .setName(recordType) + .setSinceVersion(metaDataBuilder.getVersion()) + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()); + + for (RecordMetaDataProto.Index.Builder indexBuilder : metaDataBuilder.getIndexesBuilderList()) { + // Rename any references to the type in the indexes. Note that the joined record type's index is _not_ updated + if (indexBuilder.getRecordTypeList().contains(recordType)) { + final List oldTypes = indexBuilder.getRecordTypeList(); + indexBuilder.clearRecordType(); + oldTypes.stream() + .map(type -> type.equals(recordType) ? (recordType + "__old") : type) + .forEach(indexBuilder::addRecordType); + } + } + }); + + assertFalse(validator.disallowsTypeRenames()); + assertInvalid("join constituent type changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertTrue(stricterValidator.disallowsTypeRenames()); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + + RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "join_nv2", typeBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.JoinConstituent.Builder constituent : typeBuilder.getJoinConstituentsBuilderList()) { + if (constituent.getRecordType().equals(recordType)) { + constituent.setRecordType(recordType + "__old"); + } + } + }); + validator.validate(metaData1, metaData3); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + } + + @Test + void changeJoinConstituentName() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addStrValueSelfJoin); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + if (joinedTypeBuilder.getName().equals("self_join_svi")) { + for (RecordMetaDataProto.JoinedRecordType.JoinConstituent.Builder constituentBuilder : joinedTypeBuilder.getJoinConstituentsBuilderList()) { + if (constituentBuilder.getName().equals("s1")) { + constituentBuilder.setName("x"); + } + } + joinedTypeBuilder.getJoinsBuilderList().get(0) + .setLeft("x"); + } + } + + for (RecordMetaDataProto.Index.Builder indexBuilder : metaDataBuilder.getIndexesBuilderList()) { + if (indexBuilder.getName().equals("self_join_svi$num_value_unique")) { + indexBuilder.setRootExpression(Key.Expressions.concat(Key.Expressions.field("x").nest("num_value_unique"), Key.Expressions.field("s2").nest("num_value_unique")).toKeyExpression()); + } + } + }); + + assertInvalid("join constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + laxerValidator.validate(metaData1, metaData2); + } + + @Test + void addJoinConstituent() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, TestRecords1EvolvedProto.getDescriptor(), metaDataBuilder -> { + metaDataBuilder.addRecordTypesBuilder() + .setName("AnotherRecord") + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()) + .setSinceVersion(metaDataBuilder.getVersion()); + + for (RecordMetaDataProto.JoinedRecordType.Builder joinTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + joinTypeBuilder.addJoinConstituentsBuilder() + .setName("x") + .setRecordType("AnotherRecord") + .setOuterJoined(false); + joinTypeBuilder.addJoinsBuilder() + .setLeft("l") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setRight("x") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression()); + } + }); + assertInvalid("join constituent count changed", metaData1, metaData2); + } + + @Test + void removeJoinConstituent() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + for (RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder : metaDataBuilder.getJoinedRecordTypesBuilderList()) { + if (joinedTypeBuilder.getName().equals("join_nv2")) { + joinedTypeBuilder.removeJoinConstituents(0); + joinedTypeBuilder.clearJoins(); + joinedTypeBuilder.addJoinsBuilder() + .setLeft("o") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setRight("a") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression()); + } + } + for (RecordMetaDataProto.Index.Builder index : metaDataBuilder.getIndexesBuilderList()) { + if (index.getRecordTypeList().contains("join_nv2")) { + index.setRootExpression(Key.Expressions.concat( + Key.Expressions.field("o").nest("num_value_3_indexed"), + Key.Expressions.field("a").nest("num_value_3_indexed") + ).toKeyExpression()); + } + } + }); + assertInvalid("join constituent count changed", metaData1, metaData2); + } + + @Test + void addJoinCondition() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> + // Technically, this join criterion is implied by the existing two criteria via transitivity, so + // this may need to change if we ever modify the check to account for that + joinedTypeBuilder.addJoinsBuilder() + .setLeft("o") + .setLeftExpression(Key.Expressions.field("num_value_2").toKeyExpression()) + .setLeft("a") + .setRightExpression(Key.Expressions.field("num_value_2").toKeyExpression())); + + assertInvalid("join type join count changed", metaData1, metaData2); + } + + @Test + void removeJoinCondition() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", + joinedTypeBuilder -> joinedTypeBuilder.removeJoins(0)); + assertInvalid("join type join count changed", metaData1, metaData2); + } + + @Test + void changeOuterJoinedProperty() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Assert cannot go from not outer joined to outer joined + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", + joinedTypeBuilder -> joinedTypeBuilder.getJoinConstituentsBuilder(0).setOuterJoined(true)); + assertInvalid("join constituent outer-joined property changed", metaData1, metaData2); + + // Assert cannot go from outer joined to not outer joined + final RecordMetaData metaData3 = updateMetaData(metaData1, metaDataBuilder -> metaDataBuilder.setVersion(metaData2.getVersion() + 1)); + assertInvalid("join constituent outer-joined property changed", metaData2, metaData3); + } + + @Test + void changeJoinLeftComponents() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeft("a"); + }); + assertInvalid("join changed left constituent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeftExpression(Key.Expressions.field("num_value_unique").toKeyExpression()); + }); + assertInvalid("join changed left expression", metaData1, metaData3); + } + + @Test + void changeJoinRightComponents() { + final RecordMetaData metaData1 = createEvolvedMetaData(this::addThreeWayNumValue2Join); + + final RecordMetaData metaData2 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRight("a"); + }); + assertInvalid("join changed right constituent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData1, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRightExpression(Key.Expressions.field("num_value_3_indexed").toKeyExpression()); + }); + assertInvalid("join changed right expression", metaData1, metaData3); + } + + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInJoinExpression(boolean deprecated) { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + // Change a field in MySimpleRecord used in the join + FileDescriptor renamedSimpleNum2 = mutateMessageType("MySimpleRecord", typeBuilder -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : typeBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("num_value_2")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("num_value_2__old"); + } + } + addField(typeBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_SFIXED64) + .setName("num_value_2"); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedSimpleNum2); + fieldRenameChecker.assertInvalidRenaming("join changed left expression", deprecated, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateJoinedRecordType(metaData2, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setLeftExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()); + }); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); + + // Now change the same field in MyOtherRecord + FileDescriptor renamedOtherNum2 = mutateMessageType("MyOtherRecord", renamedSimpleNum2, typeBuilder -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : typeBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("num_value_2")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("num_value_2__old"); + } + } + addField(typeBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_SFIXED64) + .setName("num_value_2"); + }); + final RecordMetaData metaData4 = replaceRecordsDescriptor(metaData3, renamedOtherNum2); + fieldRenameChecker.assertInvalidRenaming("join changed right expression", deprecated, metaData1, metaData4); + fieldRenameChecker.assertInvalidRenaming("join changed right expression", deprecated, metaData3, metaData4); + + final RecordMetaData metaData5 = mutateJoinedRecordType(metaData4, "join_nv2", joinedTypeBuilder -> { + final RecordMetaDataProto.JoinedRecordType.Join.Builder joinBuilder = joinedTypeBuilder.getJoinsBuilder(0); + joinBuilder.setRightExpression(Key.Expressions.field("num_value_2__old").toKeyExpression()); + }); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData5); + fieldRenameChecker.assertValidRenaming(deprecated, metaData3, metaData5); + } + + // UnnestedRecordTypeTests + + + // Former index tests @Test @@ -2015,14 +2389,6 @@ private void clearOptions(@Nonnull RecordMetaDataProto.Index.Builder indexProto) indexProto.clearOptions(); } - @Nonnull - private RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull Consumer updater) { - RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); - updater.accept(metaDataProtoBuilder); - metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); - return RecordMetaData.build(metaDataProtoBuilder.build()); - } - @Nonnull private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull String indexName, @Nonnull Consumer indexMutator) { return updateMetaData(metaData, metaDataProtoBuilder -> { @@ -2034,17 +2400,6 @@ private RecordMetaData mutateIndex(@Nonnull RecordMetaData metaData, @Nonnull St }); } - @Nonnull - private RecordMetaData mutateJoinedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { - return updateMetaData(metaData, metaDataProtoBuilder -> { - for (RecordMetaDataProto.JoinedRecordType.Builder typeBuilder : metaDataProtoBuilder.getJoinedRecordTypesBuilderList()) { - if (typeBuilder.getName().equals(typeName)) { - typeMutator.accept(typeBuilder); - } - } - }); - } - @Test void silentlyRemoveIndex() { RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); From e5cf8ec94ac49da92c15460171005e1b9b33197b Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 16:22:53 +0100 Subject: [PATCH 3/5] fix test that relied on dependency being available --- .../record/metadata/MetaDataEvolutionValidatorTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java index c54a50fd22..d709ef0438 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java @@ -246,7 +246,10 @@ static RecordMetaData updateMetaData(@Nonnull RecordMetaData metaData, @Nonnull RecordMetaDataProto.MetaData.Builder metaDataProtoBuilder = metaData.toProto().toBuilder(); metaDataProtoBuilder.setVersion(metaData.getVersion() + 1); updater.accept(metaDataProtoBuilder); - return RecordMetaData.build(metaDataProtoBuilder.build()); + return RecordMetaData.newBuilder() + .addDependency(TestRecords1Proto.getDescriptor()) + .setRecords(metaDataProtoBuilder.build()) + .build(); } @Nonnull From 0edf13b258e657e8d576a08e90e9778d926bdde8 Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Wed, 29 Apr 2026 19:55:25 +0100 Subject: [PATCH 4/5] add tests for unnested types modified under various conditions --- .../metadata/MetaDataEvolutionValidator.java | 8 +- .../MetaDataEvolutionValidatorTest.java | 301 ++++++++++++++++++ 2 files changed, 305 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index d1d6d3ad36..0a3909a13f 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -654,8 +654,10 @@ private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @No } } else { // Make sure we are pointing to the same parent - int oldParentPos = oldNameToConstituentPosMap.get(oldConstituent.getName()); - int newParentPos = newNameToConstituentPosMap.get(newConstituent.getName()); + final UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); + final UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); + int oldParentPos = oldNameToConstituentPosMap.get(oldParent.getName()); + int newParentPos = newNameToConstituentPosMap.get(newParent.getName()); if (oldParentPos != newParentPos) { throw new MetaDataException("nested constituent changed parent", LogMessageKeys.EXPECTED, newConstituents.get(oldParentPos).getName(), @@ -666,8 +668,6 @@ private void validateUnnestedRecordType(@Nonnull UnnestedRecordType oldType, @No // Make sure the nesting expression is still the same, possibly accounting for field renames final KeyExpression expectedNestingExpression; if (allowsAnyFieldRenames()) { - UnnestedRecordType.NestedConstituent oldParent = Objects.requireNonNull(oldConstituent.getParent()); - UnnestedRecordType.NestedConstituent newParent = Objects.requireNonNull(newConstituent.getParent()); expectedNestingExpression = RenameFieldsVisitor.renameFields(oldConstituent.getNestingExpression(), oldParent.getRecordType().getDescriptor(), newParent.getRecordType().getDescriptor()); } else { expectedNestingExpression = oldConstituent.getNestingExpression(); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java index d709ef0438..95fe15622f 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java @@ -29,6 +29,7 @@ import com.apple.foundationdb.record.TestRecords1EvolvedProto; import com.apple.foundationdb.record.TestRecords1Proto; import com.apple.foundationdb.record.TestRecords4Proto; +import com.apple.foundationdb.record.TestRecordsDoubleNestedProto; import com.apple.foundationdb.record.TestRecordsEnumProto; import com.apple.foundationdb.record.TestRecordsIdenticalTypesProto; import com.apple.foundationdb.record.TestRecordsWithHeaderProto; @@ -40,6 +41,7 @@ import com.apple.foundationdb.record.evolution.TestSplitNestedTypesProto; import com.apple.foundationdb.record.evolution.TestUnmergedNestedTypesProto; import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; +import com.apple.foundationdb.record.metadata.expressions.KeyExpression; import com.apple.foundationdb.record.provider.common.text.AllSuffixesTextTokenizer; import com.apple.foundationdb.record.provider.common.text.DefaultTextTokenizer; import com.apple.foundationdb.record.provider.common.text.PrefixTextTokenizer; @@ -67,6 +69,7 @@ import java.util.Collections; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -2105,7 +2108,305 @@ void renameFieldInJoinExpression(boolean deprecated) { // UnnestedRecordTypeTests + private void addUnnestedManyMiddleType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_many_middle"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("middle", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + metaDataBuilder.addIndex("unnest_many_middle", new Index("unnest$other_int", Key.Expressions.concat(Key.Expressions.field("outer").nest("other_int"), Key.Expressions.field("middle").nest("other_int")))); + } + + private void addUnnestedManyMiddleAndInnerType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_many_middle_and_inner"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("middle", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("inner", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor(), "middle", Key.Expressions.field("inner", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_many_middle_and_inner", new Index("unnest$other_int+foo", Key.Expressions.concat(Key.Expressions.field("middle").nest("other_int"), Key.Expressions.field("inner").nest("foo")))); + } + + private void addCrossProduceManyMiddleType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_cross_many_middle"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("m1", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("m2", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_cross_many_middle", new Index("unnest$all_other_int", Key.Expressions.concat( + Key.Expressions.field("outer").nest("other_int"), + Key.Expressions.field("m1").nest("other_int"), + Key.Expressions.field("m2").nest("other_int")))); + } + + private void addCrossProductManyMiddleAndInnerType(@Nonnull RecordMetaDataBuilder metaDataBuilder) { + final UnnestedRecordTypeBuilder unnestedBuilder = metaDataBuilder.addUnnestedRecordType("unnest_cross_many_middle_and_inner"); + unnestedBuilder.addParentConstituent("outer", metaDataBuilder.getRecordType("OuterRecord")); + unnestedBuilder.addNestedConstituent("m1", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("m2", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.getDescriptor(), "outer", Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut)); + unnestedBuilder.addNestedConstituent("inner", TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor(), "m1", Key.Expressions.field("inner", KeyExpression.FanType.FanOut)); + + metaDataBuilder.addIndex("unnest_cross_many_middle_and_inner", new Index("unnest$inner_foo_bar", + Key.Expressions.field("inner").nest(Key.Expressions.concatenateFields("foo", "bar")))); + } + + @Nonnull + private RecordMetaData mutateUnnestedRecordType(@Nonnull RecordMetaData metaData, @Nonnull String typeName, @Nonnull Consumer typeMutator) { + return updateMetaData(metaData, metaDataProtoBuilder -> { + for (RecordMetaDataProto.UnnestedRecordType.Builder typeBuilder : metaDataProtoBuilder.getUnnestedRecordTypesBuilderList()) { + if (typeBuilder.getName().equals(typeName)) { + typeMutator.accept(typeBuilder); + } + } + }); + } + + @Nonnull + private RecordMetaData createDoubleNestedMetaData(@Nonnull FDBRecordStoreTestBase.RecordMetaDataHook hook) { + RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + hook.apply(metaDataBuilder); + return metaDataBuilder.build(); + } + + @Test + void addUnnestedRecordType() { + final RecordMetaData metaData1 = RecordMetaData.build(TestRecordsDoubleNestedProto.getDescriptor()); + + final RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + addUnnestedManyMiddleType(metaDataBuilder); + final RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void dropUnnestedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecordsDoubleNestedProto.getDescriptor()); + metaDataBuilder.setVersion(metaData1.getVersion() + 1); + metaDataBuilder.addFormerIndex(new FormerIndex("unnest$other_int", metaData1.getVersion(), metaData1.getVersion() + 1, "unnest$other_int")); + final RecordMetaData metaData2 = metaDataBuilder.build(); + + validator.validate(metaData1, metaData2); + } + + @Test + void addUnnestedConstituent() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", + unnestedTypeBuilder -> unnestedTypeBuilder.addNestedConstituentsBuilder() + .setName("inner") + .setParent("middle") + .setNestingExpression(Key.Expressions.field("inner").toKeyExpression()) + .setTypeName(TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName())); + + assertInvalid("unnested type constituent count changed", metaData1, metaData2); + } + + @Test + void flipNestedConstituents() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addCrossProduceManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_cross_many_middle", unnestedTypeBuilder -> { + // Flip the order of the m2 and m1 constituents + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent m1 = unnestedTypeBuilder.getNestedConstituents(1); + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent m2 = unnestedTypeBuilder.getNestedConstituents(2); + unnestedTypeBuilder.removeNestedConstituents(2); + unnestedTypeBuilder.removeNestedConstituents(1); + unnestedTypeBuilder.addNestedConstituents(m2); + unnestedTypeBuilder.addNestedConstituents(m1); + }); + + assertInvalid("nested constituent name changed", metaData1, metaData2); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + assertInvalid("index key expression does not match required", laxerValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "unnest$all_other_int", + indexBuilder -> indexBuilder.setRootExpression(Key.Expressions.concat( + Key.Expressions.field("outer").nest("other_int"), + Key.Expressions.field("m2").nest("other_int"), + Key.Expressions.field("m1").nest("other_int") + ).toKeyExpression())); + assertInvalid("nested constituent name changed", metaData1, metaData3); + laxerValidator.validate(metaData1, metaData3); + } + + @Test + void changeParentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> { + // Change the parent from OuterRecord to middle record + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder parent = unnestedTypeBuilder.getNestedConstituentsBuilder(0); + parent.setTypeName("MiddleRecord"); + + // Update the path for the nesting key expression. It still actually points to the same nested type as before, it just takes a more interesting path to get there + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder middle = unnestedTypeBuilder.getNestedConstituentsBuilder(1); + middle.setNestingExpression(Key.Expressions.field("other_middle").nest(Key.Expressions.field("other").nest(Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut))).toKeyExpression()); + }); + + assertInvalid("unnested type parent record type changed", metaData1, metaData2); + } + + @Test + void renameParentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + FileDescriptor renamedDescriptor = mutateFile(TestRecordsDoubleNestedProto.getDescriptor(), fileBuilder -> { + DescriptorProtos.DescriptorProto outerRecord = null; + for (DescriptorProtos.DescriptorProto.Builder messageBuilder : fileBuilder.getMessageTypeBuilderList()) { + if (messageBuilder.getName().equals("OuterRecord")) { + outerRecord = messageBuilder.build(); + messageBuilder.setName("OuterRecord__old"); + } else if (messageBuilder.getName().equals(RecordMetaDataBuilder.DEFAULT_UNION_NAME)) { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageBuilder.getFieldBuilderList()) { + if (fieldBuilder.getTypeName().endsWith("OuterRecord")) { + deprecateField(fieldBuilder); + fieldBuilder.setTypeName("OuterRecord__old").setName("_OuterRecord__old"); + } + } + addField(messageBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName("OuterRecord") + .setName("_OuterRecord"); + + } + } + fileBuilder.addMessageType(Objects.requireNonNull(outerRecord)); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedDescriptor, metaDataBuilder -> { + for (RecordMetaDataProto.RecordType.Builder recordTypeBuilder : metaDataBuilder.getRecordTypesBuilderList()) { + if (recordTypeBuilder.getName().equals("OuterRecord")) { + recordTypeBuilder.setName("OuterRecord__old"); + } + } + metaDataBuilder.addRecordTypesBuilder() + .setName("OuterRecord") + .setSinceVersion(metaDataBuilder.getVersion()) + .setPrimaryKey(Key.Expressions.field("rec_no").toKeyExpression()); + }); + + assertInvalid("unnested type parent record type changed", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_many_middle", + unnestedTypeBuilder -> unnestedTypeBuilder.getNestedConstituentsBuilder(0).setTypeName("OuterRecord__old")); + validator.validate(metaData1, metaData3); + assertInvalid("record type name changed", stricterValidator, metaData1, metaData3); + } + + @Test + void changeNestedParent() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addCrossProductManyMiddleAndInnerType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_cross_many_middle_and_inner", + // Change the inner from pointing to m2 to m1 + unnestedRecordType -> unnestedRecordType.getNestedConstituentsBuilder(3).setParent("m2")); + assertInvalid("nested constituent changed parent", metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_cross_many_middle_and_inner", unnestedRecordType -> { + // Swap the positions of m1 and m2 so that now m2 is back to referring to the first middle + final List nestedConstituents = List.of( + unnestedRecordType.getNestedConstituents(0), + unnestedRecordType.getNestedConstituents(2), + unnestedRecordType.getNestedConstituents(1), + unnestedRecordType.getNestedConstituents(3) + ); + unnestedRecordType.clearNestedConstituents().addAllNestedConstituents(nestedConstituents); + }); + assertInvalid("nested constituent name changed", metaData1, metaData3); + final MetaDataEvolutionValidator laxerValidator = validator.asBuilder() + .setAllowFieldRenames(true) + .build(); + laxerValidator.validate(metaData1, metaData3); + } + + @Test + void changeNestedConstituentExpression() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> + unnestedTypeBuilder.getNestedConstituentsBuilder(1).setNestingExpression( + Key.Expressions.field("other").nest(Key.Expressions.field("outer").nest(Key.Expressions.field("many_middle", KeyExpression.FanType.FanOut))).toKeyExpression())); + + assertInvalid("nested constituent nesting expression changed", metaData1, metaData2); + } + + @Test + void changeNestedConstituentTypeName() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleAndInnerType); + + final String newFullTypeName = TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName() + "_new"; + FileDescriptor renamedFileDescriptor = mutateFile(TestRecordsDoubleNestedProto.getDescriptor(), fileBuilder -> { + for (DescriptorProtos.DescriptorProto.Builder outerMessageBuilder : fileBuilder.getMessageTypeBuilderList()) { + if (outerMessageBuilder.getName().equals("OuterRecord")) { + for (DescriptorProtos.FieldDescriptorProto.Builder outerFieldBuilder : outerMessageBuilder.getFieldBuilderList()) { + if (outerFieldBuilder.getTypeName().endsWith("InnerRecord")) { + outerFieldBuilder.setTypeName(newFullTypeName); + } + } + for (DescriptorProtos.DescriptorProto.Builder middleMessageBuilder : outerMessageBuilder.getNestedTypeBuilderList()) { + for (DescriptorProtos.FieldDescriptorProto.Builder middleFieldBuilder : middleMessageBuilder.getFieldBuilderList()) { + if (middleFieldBuilder.getTypeName().endsWith("InnerRecord")) { + middleFieldBuilder.setTypeName(newFullTypeName); + } + } + for (DescriptorProtos.DescriptorProto.Builder innerMessageBuilder : middleMessageBuilder.getNestedTypeBuilderList()) { + if (innerMessageBuilder.getName().equals("InnerRecord")) { + innerMessageBuilder.setName("InnerRecord_new"); + } + } + } + } + } + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedFileDescriptor, metaDataBuilder -> { + for (RecordMetaDataProto.UnnestedRecordType.Builder unnestedTypeBuilder : metaDataBuilder.getUnnestedRecordTypesBuilderList()) { + for (RecordMetaDataProto.UnnestedRecordType.NestedConstituent.Builder constituentBuilder : unnestedTypeBuilder.getNestedConstituentsBuilderList()) { + if (constituentBuilder.getTypeName().endsWith("InnerRecord")) { + constituentBuilder.setTypeName(newFullTypeName); + } + } + } + }); + + validator.validate(metaData1, metaData2); + } + + @ParameterizedTest + @MethodSource("deprecatedArgs") + void renameFieldInNestingExpression(boolean deprecated) { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + FileDescriptor renamedDescriptor = mutateMessageType("OuterRecord", TestRecordsDoubleNestedProto.getDescriptor(), messageType -> { + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : messageType.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("many_middle")) { + if (deprecated) { + deprecateField(fieldBuilder); + } + fieldBuilder.setName("many_middle_old"); + } + } + addField(messageType) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_REPEATED) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_MESSAGE) + .setTypeName(TestRecordsDoubleNestedProto.MiddleRecord.getDescriptor().getFullName()) + .setName("many_middle"); + }); + final RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, renamedDescriptor); + fieldRenameChecker.assertInvalidRenaming("nested constituent nesting expression changed", deprecated, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateUnnestedRecordType(metaData2, "unnest_many_middle", unnestedTypeBuilder -> + unnestedTypeBuilder.getNestedConstituentsBuilder(1).setNestingExpression(Key.Expressions.field("many_middle_old", KeyExpression.FanType.FanOut).toKeyExpression())); + fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); + } // Former index tests From 04e00b59ed9a49e8b7f203dde914628b6f898d2f Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Fri, 1 May 2026 12:20:12 +0100 Subject: [PATCH 5/5] tests for renaming synthetic record types --- .../metadata/MetaDataEvolutionValidator.java | 12 +- .../MetaDataEvolutionValidatorTest.java | 156 ++++++++++++++++++ 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java index 0a3909a13f..bd16aca3cc 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java @@ -473,10 +473,14 @@ private Map> getSyntheticTypesByRecordTypeKey(@No } private void validateSyntheticType(@Nonnull SyntheticRecordType oldType, @Nonnull SyntheticRecordType newType, @Nonnull Map typeRenames) { - if (disallowTypeRenames && !oldType.getName().equals(newType.getName())) { - throw new MetaDataException("synthetic record type name changed", - LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), - LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + if (!oldType.getName().equals(newType.getName())) { + if (disallowTypeRenames) { + throw new MetaDataException("synthetic record type name changed", + LogMessageKeys.OLD_RECORD_TYPE, oldType.getName(), + LogMessageKeys.NEW_RECORD_TYPE, newType.getName()); + } else { + typeRenames.put(oldType.getName(), newType.getName()); + } } if (!(oldType.getClass().equals(newType.getClass()))) { throw new MetaDataException("synthetic record type changed type", diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java index 95fe15622f..5d70f2d418 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java @@ -1733,6 +1733,40 @@ void dropJoinedType() { validator.validate(metaData1, metaData2); } + @Test + void renameJoinedType() { + final RecordMetaData metaData1 = createSimpleMetaData(this::addNumValue2Join); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + RecordMetaDataProto.JoinedRecordType.Builder joinedTypeBuilder = metaDataBuilder.getJoinedRecordTypesBuilder(0); + + // Add a copy with the same name but an additional join that indexes will point to + metaDataBuilder.addJoinedRecordTypes(joinedTypeBuilder.build().toBuilder() + .setRecordTypeKey(RecordKeyExpressionProto.Value.newBuilder().setLongValue(-2)) + .addJoins(RecordMetaDataProto.JoinedRecordType.Join.newBuilder() + .setLeft("l") + .setLeftExpression(Key.Expressions.field("num_value_unique").toKeyExpression()) + .setRight("r") + .setRightExpression(Key.Expressions.field("num_value_3_indexed").toKeyExpression()) + ) + ); + + // Rename the original type + joinedTypeBuilder.setName("join_nv2_old"); + }); + + assertInvalid("new index removes record type", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "joined$l.num_value_unique", indexBuilder -> + indexBuilder.clearRecordType().addRecordType("join_nv2_old")); + validator.validate(metaData1, metaData3); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData3); + } + @Test void swapJoinTypeDefinitions() { final RecordMetaData metaData1 = createSimpleMetaData(metaDataBuilder -> { @@ -2190,6 +2224,40 @@ void dropUnnestedRecordType() { validator.validate(metaData1, metaData2); } + @Test + void renameUnnestedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + RecordMetaDataProto.UnnestedRecordType.Builder unnestedTypeBuilder = metaDataBuilder.getUnnestedRecordTypesBuilder(0); + + // Make a copy with the old name and an additional constituent that old indexes will point to + metaDataBuilder.addUnnestedRecordTypes( + unnestedTypeBuilder.build().toBuilder() + .setRecordTypeKey(RecordKeyExpressionProto.Value.newBuilder().setLongValue(-2)) + .addNestedConstituents(RecordMetaDataProto.UnnestedRecordType.NestedConstituent.newBuilder() + .setName("inner") + .setParent("middle") + .setTypeName(TestRecordsDoubleNestedProto.OuterRecord.MiddleRecord.InnerRecord.getDescriptor().getFullName()) + .setNestingExpression(Key.Expressions.field("inner", KeyExpression.FanType.FanOut).toKeyExpression()) + ) + ); + // Update the name of the old one + unnestedTypeBuilder.setName("unnest_type_old"); + }); + + assertInvalid("new index removes record type", metaData1, metaData2); + final MetaDataEvolutionValidator stricterValidator = validator.asBuilder() + .setDisallowTypeRenames(true) + .build(); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData2); + + final RecordMetaData metaData3 = mutateIndex(metaData2, "unnest$other_int", indexBuilder -> + indexBuilder.clearRecordType().addRecordType("unnest_type_old")); + validator.validate(metaData1, metaData3); + assertInvalid("synthetic record type name changed", stricterValidator, metaData1, metaData3); + } + @Test void addUnnestedConstituent() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); @@ -2234,6 +2302,21 @@ void flipNestedConstituents() { laxerValidator.validate(metaData1, metaData3); } + @Test + void swapParentAndChildConstituents() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Swapping the parent and child is not allowed as the parent has to come first. But if we ever loosen that, we + // should update this test to validate that the validator rejects this change + assertThrows(MetaDataException.class, () -> mutateUnnestedRecordType(metaData1, "unnest_many_middle", unnestedTypeBuilder -> { + RecordMetaDataProto.UnnestedRecordType.NestedConstituent parentConstituent = unnestedTypeBuilder.getNestedConstituents(0); + RecordMetaDataProto.UnnestedRecordType.NestedConstituent childConstituent = unnestedTypeBuilder.getNestedConstituents(1); + unnestedTypeBuilder.clearNestedConstituents(); + unnestedTypeBuilder.addNestedConstituents(childConstituent); + unnestedTypeBuilder.addNestedConstituents(parentConstituent); + })); + } + @Test void changeParentType() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); @@ -2339,6 +2422,41 @@ void changeNestedConstituentExpression() { assertInvalid("nested constituent nesting expression changed", metaData1, metaData2); } + @Test + void evolveNestedConstituentType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Make a valid change to a field mentioned in by an unnested record type + FileDescriptor updatedDescriptor = mutateMessageType("OuterRecord", TestRecordsDoubleNestedProto.getDescriptor(), messageBuilder -> { + final DescriptorProtos.DescriptorProto.Builder middleRecordBuilder = messageBuilder.getNestedTypeBuilder(0); + addField(middleRecordBuilder) + .setLabel(DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) + .setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_STRING) + .setName("blah"); + }); + RecordMetaData metaData2 = replaceRecordsDescriptor(metaData1, updatedDescriptor, metaDataBuilder -> + metaDataBuilder.addIndexesBuilder() + .setName("unnest$blah") + .addRecordType("unnest_many_middle") + .setRootExpression(Key.Expressions.concat(Key.Expressions.field("outer").nest("other_int"), Key.Expressions.field("middle").nest("blah")).toKeyExpression()) + .setType(IndexTypes.VALUE) + .setAddedVersion(metaDataBuilder.getVersion()) + .setLastModifiedVersion(metaDataBuilder.getVersion())); + validator.validate(metaData1, metaData2); + + // Make an invalid change + FileDescriptor updatedAgainDescriptor = mutateMessageType("OuterRecord", updatedDescriptor, messageBuilder -> { + final DescriptorProtos.DescriptorProto.Builder middleRecordBuilder = messageBuilder.getNestedTypeBuilder(0); + for (DescriptorProtos.FieldDescriptorProto.Builder fieldBuilder : middleRecordBuilder.getFieldBuilderList()) { + if (fieldBuilder.getName().equals("blah")) { + fieldBuilder.setType(DescriptorProtos.FieldDescriptorProto.Type.TYPE_BYTES); + } + } + }); + RecordMetaData metaData3 = replaceRecordsDescriptor(metaData2, updatedAgainDescriptor); + assertInvalid("field type changed", metaData2, metaData3); + } + @Test void changeNestedConstituentTypeName() { final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleAndInnerType); @@ -2408,6 +2526,44 @@ void renameFieldInNestingExpression(boolean deprecated) { fieldRenameChecker.assertValidRenaming(deprecated, metaData1, metaData3); } + @Test + void unnestedTypeBecomesJoinedRecordType() { + final RecordMetaData metaData1 = createDoubleNestedMetaData(this::addUnnestedManyMiddleType); + + // Update the meta-data so that we now have a joined type instead of an unnested record type with the same key + final RecordMetaData metaData2 = updateMetaData(metaData1, metaDataBuilder -> { + final RecordMetaDataProto.UnnestedRecordType unnestedType = metaDataBuilder.getUnnestedRecordTypes(0); + metaDataBuilder.clearUnnestedRecordTypes(); + + // Copy the same constituent name, record type key, and constituent names from the unnested type so that + // any indexes defined on it pass local validation + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent outer = unnestedType.getNestedConstituents(0); + final RecordMetaDataProto.UnnestedRecordType.NestedConstituent middle = unnestedType.getNestedConstituents(1); + metaDataBuilder.addJoinedRecordTypesBuilder() + .setName(unnestedType.getName()) + .setRecordTypeKey(unnestedType.getRecordTypeKey()) + .addJoinConstituents(RecordMetaDataProto.JoinedRecordType.JoinConstituent.newBuilder() + .setName(outer.getName()) + .setRecordType(outer.getTypeName()) + .setOuterJoined(false)) + .addJoinConstituents(RecordMetaDataProto.JoinedRecordType.JoinConstituent.newBuilder() + .setName(middle.getName()) + .setRecordType("MiddleRecord") + .setOuterJoined(false)) + .addJoins(RecordMetaDataProto.JoinedRecordType.Join.newBuilder() + .setLeft(outer.getName()) + .setLeftExpression(Key.Expressions.field("middle").nest("other_int").toKeyExpression()) + .setRight(middle.getName()) + .setRightExpression(Key.Expressions.field("other_middle").nest("other_int").toKeyExpression())); + }); + + assertInvalid("synthetic record type changed type", metaData1, metaData2); + + // Should also be a problem if we change back + final RecordMetaData metaData3 = updateMetaData(metaData1, metaDataBuilder -> metaDataBuilder.setVersion(metaData2.getVersion() + 1)); + assertInvalid("synthetic record type changed type", metaData2, metaData3); + } + // Former index tests @Test