Skip to content

Support nullable and non-nullable types with same name in the same template #3658

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
pengpeng-lu wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Support nullable and non-nullable types with same name in the same template #3658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2e00001
save
pengpeng-lu Sep 25, 2025
3082449
save
pengpeng-lu Sep 29, 2025
cb96a99
save
pengpeng-lu Sep 30, 2025
f94e628
test work
pengpeng-lu Oct 2, 2025
9eaf300
add test
pengpeng-lu Oct 2, 2025
d64c340
remove unnecessary methods
pengpeng-lu Oct 2, 2025
de29718
save
pengpeng-lu Oct 3, 2025
7804c64
fix tests and style
pengpeng-lu Oct 3, 2025
6af4023
save
pengpeng-lu Oct 16, 2025
c7b7243
add yaml files
pengpeng-lu Oct 16, 2025
34a88eb
remove an empty file
pengpeng-lu Oct 16, 2025
0d68610
remove
pengpeng-lu Oct 16, 2025
fdd0367
fix test
pengpeng-lu Oct 16, 2025
023d6ae
fix mixed mode test
pengpeng-lu Oct 16, 2025
5505514
merge main
pengpeng-lu Dec 4, 2025
4f2d738
two maps
pengpeng-lu Dec 4, 2025
759e93c
style
pengpeng-lu Dec 4, 2025
2de10ad
style
pengpeng-lu Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 101 additions & 25 deletions ...rc/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@
import com.google.common.base.Verify;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.ImmutableBiMap;
Copy link
Collaborator

@alecgrieser alecgrieser Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting here some unit tests that should be added to TypeRepositoryTest:

  1. A simple unit test adding both a nullable and non-nullable variant of the same type with the same name
  2. A test like addSameTypeMultipleTimesShouldNotCreateMultipleMessageTypes, but it adds a nullable and non-nullable variant of the same type
  3. Ideally, a variant of the above with an enum
  4. Check that if we can't add two copies of the same type (even if they have different nullability) with different names. (I think this one is handled correctly by the code already.)
  5. Check what happens if we add two types with the same name and different nullability and different contents. This should be an error, but I think it's currently not handled correctly
  6. Creating a builder using one nullability of a type, and then calling getProtoTypeName or newMessageBuilder with the other nullability. I believe this currently throws an error, though I'm not sure it needs to
  7. Something like addPrimitiveTypeIsNotAllowed, but also including Any, Null, None, and Relation types. It would be good to also update addPrimitiveTypeIsNotAllowed while you're there, because the assertion is off. That doesn't throw anymore, so having the catch is confusing and muddies what the assertion actually is

Maybe these should be in some schema file, but it is probably going to be easier to get the coverage in unit testing

import com.google.protobuf.DescriptorProtos;
import com.google.protobuf.DescriptorProtos.FileDescriptorProto;
import com.google.protobuf.DescriptorProtos.FileDescriptorSet;
Expand All @@ -50,7 +49,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
Expand Down Expand Up @@ -85,14 +83,25 @@
@Nonnull
private final Map<String, EnumDescriptor> enumDescriptorMapShort = new LinkedHashMap<>();

/**
* BiMap storing nullable types and their protobuf names.
* Only types with isNullable() = true are stored here.
*/
@Nonnull
private final BiMap<Type, String> nullableTypeToNameMap;

/**
* BiMap storing non-nullable types and their protobuf names.
* Only types with isNullable() = false are stored here.
*/
@Nonnull
private final Map<Type, String> typeToNameMap;
private final BiMap<Type, String> nonNullableTypeToNameMap;
Copy link
Collaborator

@alecgrieser alecgrieser Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think splitting the maps has made the logic in registerTypeToTypeNameMapping and addTypeIfNeeded a little too complicated. My suggestion was actually to keep everything in one map, but to only put the nullable variants into it. Then I think the only thing that needs to change is that anything that accesses the map needs to call nullable() to create the nullable variant before it goes to look things up from the map.


@Nonnull
public static TypeRepository empty() {
FileDescriptorSet.Builder resultBuilder = FileDescriptorSet.newBuilder();
try {
return new TypeRepository(resultBuilder.build(), Maps.newHashMap());
return new TypeRepository(resultBuilder.build(), HashBiMap.create(), HashBiMap.create());
} catch (final DescriptorValidationException e) {
throw new IllegalStateException(e);
}
Expand Down Expand Up @@ -123,9 +132,9 @@
}

@Nonnull
public static TypeRepository parseFrom(@Nonnull final byte[] schemaDescBuf) throws DescriptorValidationException, IOException {
return new TypeRepository(FileDescriptorSet.parseFrom(schemaDescBuf), Maps.newHashMap());
return new TypeRepository(FileDescriptorSet.parseFrom(schemaDescBuf), HashBiMap.create(), HashBiMap.create());
}

Check warning on line 137 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java#L135-L137 

[Test Gap] Changed method `parseFrom` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-record-layer-core%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frecord%2Fquery%2Fplan%2Fcascades%2Ftyping%2FTypeRepository.java?coverage-mode=test-gap&t=FORK_MR%2F3658%2Fpengpeng-lu%2Fsame_name%3AHEAD&selection=char-4709-4960&merge-request=FoundationDB%2Ffdb-record-layer%2F3658

/**
* Creates a new dynamic message builder for the given message type.
Expand All @@ -150,8 +159,7 @@
*/
@Nullable
public DynamicMessage.Builder newMessageBuilder(@Nonnull final Type type) {
final String msgTypeName = Preconditions.checkNotNull(typeToNameMap.get(type));
Objects.requireNonNull(msgTypeName);
final String msgTypeName = getProtoTypeName(type);
return newMessageBuilder(msgTypeName);
}

Expand All @@ -162,8 +170,13 @@
*/
@Nonnull
public String getProtoTypeName(@Nonnull final Type type) {
final String typeName = Preconditions.checkNotNull(typeToNameMap.get(type));
return Objects.requireNonNull(typeName);
final String typeName;
if (type.isNullable()) {
typeName = nullableTypeToNameMap.get(type);
} else {
typeName = nonNullableTypeToNameMap.get(type);
}
return Preconditions.checkNotNull(typeName, "Type not found in repository: %s", type);
}

/**
Expand Down Expand Up @@ -305,7 +318,8 @@
}

private TypeRepository(@Nonnull final FileDescriptorSet fileDescSet,
@Nonnull final Map<Type, String> typeToNameMap) throws DescriptorValidationException {
@Nonnull final BiMap<Type, String> nullableTypeToNameMap,
@Nonnull final BiMap<Type, String> nonNullableTypeToNameMap) throws DescriptorValidationException {
this.fileDescSet = fileDescSet;
Map<String, FileDescriptor> fileDescMap = init(fileDescSet);

Expand All @@ -327,7 +341,8 @@
enumDescriptorMapShort.remove(enumName);
}

this.typeToNameMap = ImmutableMap.copyOf(typeToNameMap);
this.nullableTypeToNameMap = ImmutableBiMap.copyOf(nullableTypeToNameMap);
this.nonNullableTypeToNameMap = ImmutableBiMap.copyOf(nonNullableTypeToNameMap);
}

@SuppressWarnings("java:S3776")
Expand Down Expand Up @@ -436,13 +451,15 @@
public static class Builder {
private @Nonnull final FileDescriptorProto.Builder fileDescProtoBuilder;
private @Nonnull final FileDescriptorSet.Builder fileDescSetBuilder;
private @Nonnull final BiMap<Type, String> typeToNameMap;
private @Nonnull final BiMap<Type, String> nullableTypeToNameMap;
private @Nonnull final BiMap<Type, String> nonNullableTypeToNameMap;

private Builder() {
fileDescProtoBuilder = FileDescriptorProto.newBuilder();
fileDescProtoBuilder.addAllDependency(DEPENDENCIES.stream().map(FileDescriptor::getFullName).collect(Collectors.toList()));
fileDescSetBuilder = FileDescriptorSet.newBuilder();
typeToNameMap = HashBiMap.create();
nullableTypeToNameMap = HashBiMap.create();
nonNullableTypeToNameMap = HashBiMap.create();
}

@Nonnull
Expand All @@ -451,7 +468,7 @@
resultBuilder.addFile(fileDescProtoBuilder.build());
resultBuilder.mergeFrom(this.fileDescSetBuilder.build());
try {
return new TypeRepository(resultBuilder.build(), typeToNameMap);
return new TypeRepository(resultBuilder.build(), nullableTypeToNameMap, nonNullableTypeToNameMap);
} catch (final DescriptorValidationException dve) {
throw new IllegalStateException("validation should not fail", dve);
}
Expand All @@ -471,20 +488,50 @@

@Nonnull
public Builder addTypeIfNeeded(@Nonnull final Type type) {
if (!typeToNameMap.containsKey(type)) {
// Type.Null is special - it can only be nullable and has no non-nullable variant
if (type instanceof Type.Null) {
if (!nullableTypeToNameMap.containsKey(type)) {
type.defineProtoType(this);
}
return this;
}
Comment on lines +491 to +497
Copy link
Collaborator

@alecgrieser alecgrieser Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Relations and Any are also special in that they also have a fixed nullability. (Relations are always non-null, and Any is always nullable.)

It's a bit weird, as none of those types actually override defineProtoType, so they won't end up adding anything to the type repository anyway. But I suppose it doesn't hurt to make sure we cover for them. Potentially a method like canonicalizeNullability(Type) would be useful that returned (1) a non-nullable Relation and None, and (2) a nullable value for everything else. Then that would be the type we probed for.


// Type.None is special - it can only be non-nullable and has no nullable variant
if (type.getTypeCode() == Type.TypeCode.NONE) {
if (!nonNullableTypeToNameMap.containsKey(type)) {
type.defineProtoType(this);
}
return this;
}


final BiMap<Type, String> targetMap = type.isNullable() ? nullableTypeToNameMap : nonNullableTypeToNameMap;
final BiMap<Type, String> oppositeMap = type.isNullable() ? nonNullableTypeToNameMap : nullableTypeToNameMap;

if (!targetMap.containsKey(type)) {
// Check if the opposite nullability variant exists and get its protobuf name
final Type oppositeNullabilityType = type.isNullable() ? type.notNullable() : type.withNullability(true);
Copy link
Collaborator

@alecgrieser alecgrieser Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified to:

final Type oppositeNullabilityType = type.withNullability(!type.isNullable());

String existingProtoName = oppositeMap.get(oppositeNullabilityType);

if (existingProtoName != null) {
// Reuse the same protobuf name for the type with different nullability
targetMap.put(type, existingProtoName);
return this;
}

// Standard case: define the protobuf type
type.defineProtoType(this);
}
return this;
}

@Nonnull
public Optional<String> getTypeName(@Nonnull final Type type) {
return Optional.ofNullable(typeToNameMap.get(type));
}

@Nonnull
public Optional<Type> getTypeByName(@Nonnull final String name) {
return Optional.ofNullable(typeToNameMap.inverse().get(name));
if (type.isNullable()) {
return Optional.ofNullable(nullableTypeToNameMap.get(type));
} else {
return Optional.ofNullable(nonNullableTypeToNameMap.get(type));
}
}

@Nonnull
Expand All @@ -501,8 +548,37 @@

@Nonnull
public Builder registerTypeToTypeNameMapping(@Nonnull final Type type, @Nonnull final String protoTypeName) {
Verify.verify(!typeToNameMap.containsKey(type));
typeToNameMap.put(type, protoTypeName);
final BiMap<Type, String> targetMap = type.isNullable() ? nullableTypeToNameMap : nonNullableTypeToNameMap;
final BiMap<Type, String> oppositeMap = type.isNullable() ? nonNullableTypeToNameMap : nullableTypeToNameMap;

Check warning on line 552 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java#L552 

Move the declaration of "oppositeMap" closer to the code that uses it
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?t=FORK_MR%2F3658%2Fpengpeng-lu%2Fsame_name%3AHEAD&id=FC0E282895341A8BFB60529DA039A5A7

final String existingTypeName = targetMap.get(type);
if (existingTypeName != null) {
// Type already registered, verify same protobuf name
Verify.verify(existingTypeName.equals(protoTypeName),
"Type %s is already registered with name %s, cannot register with different name %s",
type, existingTypeName, protoTypeName);
return this;
}

// Check if the opposite nullability variant uses this protobuf name
final Type oppositeNullabilityType = type.isNullable() ? type.notNullable() : type.withNullability(true);
final String oppositeExistingName = oppositeMap.get(oppositeNullabilityType);
if (oppositeExistingName != null && oppositeExistingName.equals(protoTypeName)) {
// Both nullable and non-nullable variants can share the same protobuf type name
targetMap.put(type, protoTypeName);
return this;
}

// Check if this protobuf name is already used by a different structural type
final Type existingTypeForName = targetMap.inverse().get(protoTypeName);
if (existingTypeForName != null && !existingTypeForName.equals(type)) {
throw new IllegalArgumentException(String.format(
Comment on lines +573 to +575
Copy link
Collaborator

@alecgrieser alecgrieser Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is actually already handled by targetMap.put, though I'm not necessarily opposed to overriding the error message.

What I think we are missing here is coverage of the case:

  1. You insert a (let's say) nullable type t with a name x
  2. You insert a type u that is not nullable and !u.nullable().equals(t) but you also assign it the name x

I think you'd need to add a check that oppositeMap.inverse().get(protoTypeName) was either null or equal to oppositeNullabilityType. I also think that this would naturally get covered if there were just one BiMap with a canonicalized nullability

"Name %s is already registered with a different type %s, cannot register with type %s",
protoTypeName, existingTypeForName, type));
}

// Standard case: new type with new name (or same name as opposite nullability)
targetMap.put(type, protoTypeName);
return this;
}

Expand All @@ -515,7 +591,7 @@
@Nonnull
public Optional<String> defineAndResolveType(@Nonnull final Type type) {
addTypeIfNeeded(type);
return Optional.ofNullable(typeToNameMap.get(type));
return getTypeName(type);
}

@Nonnull
Expand Down
2 changes: 1 addition & 1 deletion ...src/test/java/com/apple/foundationdb/relational/recordlayer/query/StandardQueryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,7 +1219,7 @@ void testNamingStructsDifferentTypesThrows() throws Exception {
try (var statement = ddl.setSchemaAndGetConnection().createStatement()) {
statement.executeUpdate("insert into t1 values (42, 100, 500, 101)");
final var message = Assertions.assertThrows(SQLException.class, () -> statement.execute("select struct asd (a, 42, struct def (b, c), struct def(b, c, a)) as X from t1")).getMessage();
Assertions.assertTrue(message.contains("value already present: DEF")); // we could improve this error message.
Assertions.assertTrue(message.contains("Name DEF is already registered with a different type")); // we could improve this error message.
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions yaml-tests/src/test/java/YamlIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ public void standardTestsWithProto(YamlTest.Runner runner) throws Exception {
runner.runYamsql("standard-tests-proto.yamsql");
}

@TestTemplate
public void structTypeNullabilityVariants(YamlTest.Runner runner) throws Exception {
runner.runYamsql("struct-type-nullability-variants.yamsql");
}

@TestTemplate
public void subqueryTests(YamlTest.Runner runner) throws Exception {
runner.runYamsql("subquery-tests.yamsql");
Expand Down
Loading