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

pengpeng-lu · 2025-10-02T23:29:06Z

Modify TypeRepository to allow registering 2 types (only different in nullability) in the same schema template.
Use Case:
If we have struct A, and array of A in the same template, A is nullable, while A in the array is non-nullable. This should be allowed.

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

alecgrieser · 2025-12-04T11:40:27Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+         */
+        @Nullable
+        private Type findCanonicalTypeForStructure(@Nonnull final Type type) {
+            for (Map.Entry<Type, String> entry : typeToNameMap.entrySet()) {


This now introduces an Θ(n) step into creating the type repository, which would mean that creating a repository with n) types is now Θ(n²).

Did you consider an alternative design where we enforce that only the notNullable (or nullable--it doesn't matter except for certain edge cases like Type.Null) variants of each type are kept in the TypeRepository? From my reading of the type repository, we don't actually need to keep both the nullable and non-nullable variants of the type at all, as we only use it to associate a type to a Protobuf descriptor, and those are the same for the two different variants.

I think the basic shape of the solution would look something like:

Modify the builder to store only the (non) nullable version of each type

Wrap the access paths to the typeToName bi-map with logic to first transform the type to its (non) nullable variant

yaml-tests/src/test/resources/struct-type-nullability-variants.yamsql

yaml-tests/src/test/java/YamlIntegrationTests.java

alecgrieser · 2025-12-05T10:36:45Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+
+            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);


I think this could be simplified to:

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

yaml-tests/src/test/resources/struct-type-nullability-variants.yamsql

alecgrieser · 2025-12-05T10:54:01Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+     */
    @Nonnull
-    private final Map<Type, String> typeToNameMap;
+    private final BiMap<Type, String> nonNullableTypeToNameMap;


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.

alecgrieser · 2025-12-05T11:02:04Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+            // 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;
+            }


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.

alecgrieser · 2025-12-05T11:08:08Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+            final Type existingTypeForName = targetMap.inverse().get(protoTypeName);
+            if (existingTypeForName != null && !existingTypeForName.equals(type)) {
+                throw new IllegalArgumentException(String.format(


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:

You insert a (let's say) nullable type t with a name x

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

alecgrieser · 2025-12-05T11:34:09Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

 import com.google.common.collect.HashBiMap;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Maps;
+import com.google.common.collect.ImmutableBiMap;


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

A simple unit test adding both a nullable and non-nullable variant of the same type with the same name

A test like addSameTypeMultipleTimesShouldNotCreateMultipleMessageTypes, but it adds a nullable and non-nullable variant of the same type

Ideally, a variant of the above with an enum

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.)

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

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

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

yaml-tests/src/test/resources/struct-type-nullability-variants.yamsql

alecgrieser

Great! I had a few minor things that could be addressed if you feel like it, but I like the way this has shaped up!

alecgrieser · 2025-12-09T17:32:14Z

...e/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/TypeRepository.java

+        if (type.getTypeCode() == Type.TypeCode.RELATION || type.getTypeCode() == Type.TypeCode.NONE) {
+            return type.notNullable();
+        } else {
+            return type.withNullability(true);


minor nit:

Suggested change

return type.withNullability(true);

return type.nullable();

alecgrieser · 2025-12-09T17:37:33Z

...core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/TypeRepositoryTest.java

+        }
+
+        // Test special types that should not be allowed
+        List<Type> unsupportedTypes = List.of(


Is there a reason that primitives and unsupported types are being separated out by this test case? Unless I'm missing something, it looks like the asserts are the same. Is it just that the primitive types put their type inside the loop?

It would be nice to have the asserts shared between the two test cases. One way to do this would be to have this be a parameterized test based on the Type, and then have a helper method generate the different primitive and "unsupported" types. But it would also be fine just to collect them in a list, and then have them all call the asserts. It may also make sense to add (1) both nullable and non-null primitives and (2) Type.UUID_NULL_INSTANCE and Type.UUID_NON_NULL_INSTANCE

alecgrieser · 2025-12-09T17:38:08Z

...core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/TypeRepositoryTest.java

+        List<Type> unsupportedTypes = List.of(
+                Type.any(),                           // ANY type
+                Type.nullType(),                      // NULL type
+                new Type.None(),                      // NONE type


minor:

Suggested change

new Type.None(), // NONE type

Type.NONE, // NONE type

github-actions · 2025-12-10T00:51:33Z

📊 Metrics Diff Analysis Report

Summary

New queries: 5
Dropped queries: 0
Plan changed + metrics changed: 0
Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

New queries: Queries added in this PR
Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
Plan changed + metrics changed: The query plan has changed along with planner metrics.
Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

New Queries

Count of new queries by file:

yaml-tests/src/test/resources/struct-type-nullability-variants.metrics.yaml: 5

pengpeng-lu added 13 commits September 25, 2025 16:43

save

2e00001

save

3082449

save

cb96a99

test work

f94e628

add test

9eaf300

remove unnecessary methods

d64c340

save

de29718

fix tests and style

7804c64

save

6af4023

add yaml files

c7b7243

remove an empty file

34a88eb

remove

0d68610

fix test

fdd0367

pengpeng-lu added the enhancement New feature or request label Oct 16, 2025

fix mixed mode test

023d6ae

pengpeng-lu requested a review from hatyo October 17, 2025 15:29

arnaud-lacurie requested a review from alecgrieser November 27, 2025 15:31

alecgrieser requested changes Dec 4, 2025

View reviewed changes

pengpeng-lu added 4 commits December 4, 2025 11:53

merge main

5505514

two maps

4f2d738

style

759e93c

style

2de10ad

alecgrieser requested changes Dec 5, 2025

View reviewed changes

pengpeng-lu added 4 commits December 8, 2025 14:17

comments

df3ace4

style

dc794ed

Merge branch 'main' into same_name

f9d1b0a

revert unnecessary change

7a77baa

alecgrieser approved these changes Dec 9, 2025

View reviewed changes

nit

a21980a

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

Are you sure you want to change the base?

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

Conversation

pengpeng-lu commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alecgrieser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 10, 2025

📊 Metrics Diff Analysis Report

Summary

New Queries

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pengpeng-lu commented Oct 2, 2025 •

edited

Loading