From e67b1b3fa3198fde85dfe4920d4a4f84f28446ff Mon Sep 17 00:00:00 2001 From: Mike McMahon Date: Wed, 4 Feb 2026 15:32:11 -0800 Subject: [PATCH 1/3] Add a test of Proto3 with explicit presence. --- .../query/FDBRecordStoreNullQueryTest.java | 64 +++++++++++++++++++ .../src/test/proto/test_records_nulls_3.proto | 19 ++++++ 2 files changed, 83 insertions(+) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreNullQueryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreNullQueryTest.java index 0d3e5a8190..1cc50d088a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreNullQueryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreNullQueryTest.java @@ -25,6 +25,7 @@ import com.apple.foundationdb.record.RecordMetaDataBuilder; import com.apple.foundationdb.record.TestRecordsNulls2Proto; import com.apple.foundationdb.record.TestRecordsNulls3Proto; +import com.apple.foundationdb.record.TestRecordsNulls3ExplicitProto; import com.apple.foundationdb.record.TestRecordsTupleFieldsProto; import com.apple.foundationdb.record.TupleFieldsProto; import com.apple.foundationdb.record.metadata.Key; @@ -108,6 +109,10 @@ protected static RecordMetaData proto3NestedMetaData() { return metaData.getRecordMetaData(); } + protected static RecordMetaData proto3ExplicitMetaData() { + return RecordMetaData.newBuilder().setRecords(TestRecordsNulls3ExplicitProto.getDescriptor()).getRecordMetaData(); + } + @FunctionalInterface interface RecordBuilder { M build(@Nonnull String name, @Nullable Integer intValue, @Nullable String stringValue); @@ -175,6 +180,18 @@ protected static DynamicMessage buildRecord3Dynamic(@Nonnull String name, @Nulla return builder.build(); } + protected static TestRecordsNulls3ExplicitProto.MyNullRecord buildRecord3Explicit(@Nonnull String name, @Nullable Integer intValue, @Nullable String stringValue) { + TestRecordsNulls3ExplicitProto.MyNullRecord.Builder builder = TestRecordsNulls3ExplicitProto.MyNullRecord.newBuilder(); + builder.setName(name); + if (intValue != null) { + builder.setIntValue(intValue); + } + if (stringValue != null) { + builder.setStringValue(stringValue); + } + return builder.build(); + } + protected void saveTestRecords(@Nonnull RecordBuilder builder) { recordStore.saveRecord(builder.build("empty", null, null)); recordStore.saveRecord(builder.build("default", 0, "")); @@ -365,6 +382,53 @@ public void testProto3Nested() { } } + // Explicit presence ("optional") correctly distguishes as well (results same as proto2). + @Test + public void testProto3Explicit() { + try (FDBRecordContext context = openContext()) { + createOrOpenRecordStore(context, proto3ExplicitMetaData()); + saveTestRecords(FDBRecordStoreNullQueryTest::buildRecord3Explicit); + + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("int_value").equalsValue(2)) + .build()), + is(Collections.singletonList("two"))); + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("string_value").equalsValue("B")) + .build()), + is(Collections.singletonList("two"))); + + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("int_value").isNull()) + .build()), + is(Collections.singletonList("empty"))); + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("int_value").equalsValue(0)) + .build()), + is(Collections.singletonList("default"))); + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("string_value").isNull()) + .build()), + is(Collections.singletonList("empty"))); + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setFilter(Query.field("string_value").equalsValue("")) + .build()), + is(Collections.singletonList("default"))); + + assertThat(executeQuery(RecordQuery.newBuilder() + .setRecordType("MyNullRecord") + .setSort(Key.Expressions.field("int_value")) + .build()), + is(Arrays.asList("empty", "minus", "default", "one", "two"))); + } + } + @Test public void testCompareSerialization() throws Exception { final TestRecordsNulls2Proto.MyNullRecord emptyProto2 = buildRecord2("record", null, null); diff --git a/fdb-record-layer-core/src/test/proto/test_records_nulls_3.proto b/fdb-record-layer-core/src/test/proto/test_records_nulls_3.proto index 52d0426200..0f5d532bc8 100644 --- a/fdb-record-layer-core/src/test/proto/test_records_nulls_3.proto +++ b/fdb-record-layer-core/src/test/proto/test_records_nulls_3.proto @@ -1,3 +1,22 @@ +/* + * test_records_nulls_3.proto + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2018 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ syntax = "proto3"; package com.apple.foundationdb.record.testnulls3; From 22e28f812d5d2be93a89de3b9a191cb4320cc5e3 Mon Sep 17 00:00:00 2001 From: Mike McMahon Date: Wed, 4 Feb 2026 15:53:53 -0800 Subject: [PATCH 2/3] Update meta-data evolution validator to check presence rather than optional. Also, changing this in either direction is problematic as to whether default values are indexed as nulls. --- .../metadata/MetaDataEvolutionValidator.java | 7 ++-- .../MetaDataEvolutionValidatorTest.java | 15 +++++--- .../proto/test_records_nulls_3_explicit.proto | 37 +++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 fdb-record-layer-core/src/test/proto/test_records_nulls_3_explicit.proto 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 0ce214a7dd..ef306dc558 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 @@ -274,13 +274,12 @@ private void validateField(@Nonnull FieldDescriptor oldFieldDescriptor, @Nonnull if (oldFieldDescriptor.isRequired() && !newFieldDescriptor.isRequired()) { throw new MetaDataException("required field is no longer required", LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName()); - } else if (oldFieldDescriptor.isOptional() && !newFieldDescriptor.isOptional()) { - // TODO: In theory, optional -> repeated is okay, but only if the field is not indexed - throw new MetaDataException("optional field is no longer optional", - LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName()); } else if (oldFieldDescriptor.isRepeated() && !newFieldDescriptor.isRepeated()) { throw new MetaDataException("repeated field is no longer repeated", LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName()); + } else if (oldFieldDescriptor.hasPresence() != newFieldDescriptor.hasPresence()) { + throw new MetaDataException("field changed whether default values are stored if set explicitly", + LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName()); } if (oldFieldDescriptor.getType().equals(FieldDescriptor.Type.ENUM)) { validateEnum(newFieldDescriptor.getName(), oldFieldDescriptor.getEnumType(), newFieldDescriptor.getEnumType()); 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 c888cd0027..c5bbaad8a2 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 @@ -857,12 +857,17 @@ public void fieldLabelChanged() { DescriptorProtos.FieldDescriptorProto.Label.LABEL_REQUIRED ); for (int i = 0; i < labels.size(); i++) { - final int itr = i; - final DescriptorProtos.FieldDescriptorProto.Label label = labels.get(itr); - final String labelText = label.name().substring(label.name().indexOf('_') + 1).toLowerCase(Locale.ROOT); - final String errMsg = labelText + " field is no longer " + labelText; + final DescriptorProtos.FieldDescriptorProto.Label label = labels.get(i); + final String errMsg; + if (label == DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) { + errMsg = "field changed whether default values are stored if set explicitly"; + } else { + final String labelText = label.name().substring(label.name().indexOf('_') + 1).toLowerCase(Locale.ROOT); + errMsg = labelText + " field is no longer " + labelText; + } + final DescriptorProtos.FieldDescriptorProto.Label newLabel = labels.get((i + 1) % labels.size()); FileDescriptor updatedFile = mutateField("MySimpleRecord", "str_value_indexed", oldFile, - field -> field.setLabel(labels.get((itr + 1) % labels.size()))); + field -> field.setLabel(newLabel)); assertInvalid(errMsg, oldFile, updatedFile); oldFile = updatedFile; diff --git a/fdb-record-layer-core/src/test/proto/test_records_nulls_3_explicit.proto b/fdb-record-layer-core/src/test/proto/test_records_nulls_3_explicit.proto new file mode 100644 index 0000000000..1cd85aec2e --- /dev/null +++ b/fdb-record-layer-core/src/test/proto/test_records_nulls_3_explicit.proto @@ -0,0 +1,37 @@ +/* + * test_records_nulls_3_explicit.proto + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2026 Apple Inc. and the FoundationDB project authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +syntax = "proto3"; + +package com.apple.foundationdb.record.testnulls3explicit; + +option java_package = "com.apple.foundationdb.record"; +option java_outer_classname = "TestRecordsNulls3ExplicitProto"; + +import "record_metadata_options.proto"; + +message MyNullRecord { + optional string name = 1 [(field).primary_key = true]; + optional string string_value = 2 [(field).index = {}]; + optional int32 int_value = 3 [(field).index = {}]; +} + +message RecordTypeUnion { + optional MyNullRecord _MyNullRecord = 1; +} From 24ef12eb452346d163c6197d8abd7146bbec7ba4 Mon Sep 17 00:00:00 2001 From: Mike McMahon Date: Wed, 4 Feb 2026 16:52:17 -0800 Subject: [PATCH 3/3] Try to trigger build