Revisit enum DDL semantics w.r.t. proto3 enum specs

[hatyo]

When we create an Enum type in SQL, the DDL starts assigning the enumeration values with numbers starting from 0

fdb-record-layer/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/DdlVisitor.java

Lines 187 to 190 in 5a65936

	final List<DataType.EnumType.EnumValue> enumValues = new ArrayList<>(ctx.STRING_LITERAL().size());
	for (int i = 0; i < ctx.STRING_LITERAL().size(); i++) {
	enumValues.add(DataType.EnumType.EnumValue.of(Assert.notNullUnchecked(getDelegate().normalizeString(ctx.STRING_LITERAL(i).getText())), i));
	}

which could have unexpected side-effects considering how proto3 attributes special semantics to the enumeration value with number 0.

Enum Default Value
The default value for the SearchRequest.corpus field is CORPUS_UNSPECIFIED because that is the first value defined in the enum.
In proto3, the first value defined in an enum definition must have the value zero and should have the name ENUM_TYPE_NAME_UNSPECIFIED or ENUM_TYPE_NAME_UNKNOWN. This is because:
There must be a zero value, so that we can use 0 as a numeric default value.
The zero value needs to be the first element, for compatibility with the proto2 semantics where the first enum value is the default unless a different value is explicitly specified.
It is also recommended that this first, default value have no semantic meaning other than “this value was unspecified”.

[hatyo]

cc @alecgrieser

[alecgrieser]

Interesting find. When enums were first added to the DDL (in 803d62c), I believe that the DDL actually did start its values from:

fdb-record-layer/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/TypingContext.java

Lines 142 to 151 in 803d62c

	List<Type.Enum.EnumValue> enumValues = new ArrayList<>(enumDefinition.values.size());
	// Start counting enums at 1. In proto3 syntax, there must be a default enum value at position 0,
	// but this value ends up being easy to confuse with an unset enum value. In the future, if we
	// want to switch schema templates to use proto3 syntax, we can introduce an "UNSET" value
	// at position 0 that we treat as null/unset and preserve compatibility with data on disk.
	int valueCounter = 1;
	for (String enumValue : enumDefinition.values) {
	enumValues.add(new Type.Enum.EnumValue(enumValue, valueCounter++));
	}
	Type.Enum enumType = new ReferentialEnum(false, enumValues, enumDefinition.getName());

As you can see by the comment, it was for similar reasons to the ones being laid out here.

One other historical note: if the user is using proto3 in a mode where a field is indistinguishably unset or set to the default, then an enum value of 0 is somewhat dangerous. This is because hasField will return false for that field if set to the default, which means that we'll end up indexing null. This can lead to query failures if we, for example, look for all values in an index where we set the field to 0 and we should instead be looking for null. This can be addressed at the DDL layer by always generating a proto message with real has semantics (e.g., proto2 or the appropriate edition), which we probably want to do anyway. But it's another reason why using enum value 0 is a bit dangerous. (We could also consider a different approach to handling hasField when the protobuf field does not admit an unset/set to default distinction. We currently always assume that those are null, but we could instead inspect the descriptor and make a determination based on that. That's technically a backwards incompatible change for any data in that state, though, so we'd need to tread carefully.)

Overall, I'm pretty supportive of using 1 as the first "real" value, and then either leaving 0 blank (which is not compatible with proto3, but there's a compatibility mode that was added in the proto "editions" framework) or inserting a special system value meaning "unset", if there's some keyword that we can use to avoid conflicting with user-defined enum keys.