Reintroduce translation of identifiers to Protobuf compliant names #3736
Conversation
This reintroduces the work that was reverted (namely FoundationDB#3696 and FoundationDB#3706; see also FoundationDB#3726). It mainly copies the original PR for the translation work itself, though it takes a different approach to actually performing the translation. Perhaps most notably, this purposefully omits translation of function, view, and index names--essentially, any item that is not serialized into a Protobuf identifier is skipped. That means that we only worry about type (including table, struct, and enum names), enum values, and struct/table fields. From an implementation this perspective, this moves the translation of the names into the areas that are designed to interface between the `Type` system of the planner and the Protobuf-based system of Record Layer core. This is then stored in the `Type` information that is associated with the plan. At the moment, we cheat in a few places and require that the type's fields are always the result of applying the translation function to the display name, or vice versa. However, it allows us to think about allowing custom field names in the future. In a world where we only use Protobuf for storage but have a different mechanism for keeping track of fields in the runtime, we'd need a similar mechanism, though it may take the form of something like a field name to ordinal position mapping, say. This borrows and expands the tests from #3969 and FoundationDB#3706. In particular, it adds additional tests around enums and view and function definitions with non-compliant names, which now work.
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. New QueriesCount of new queries by file:
|
|
The inconsistency between table names, and the corresponding (qualified) field names seems confusing. Would it be possible to fix it? (see below, If I am not mistaken, that shouldn't be difficult, maybe we can just print the storage name of the 
|
|
Yes, we could address that. There are two ways you could address that:
I actually kind of prefer the latter. One thing I'm not super happy about is that the user still needs to be a bit careful about which One reason to keep the user-visible names during |
|
I'll go ahead and address the table name display inconsistency in a follow up PR. |
2 participants
This reintroduces the work that was reverted (namely #3696 and #3706; see also #3726). It mainly copies the original PR for the translation work itself, though it takes a different approach to actually performing the translation.
Perhaps most notably, this purposefully omits translation of function, view, and index names--essentially, any item that is not serialized into a Protobuf identifier is skipped. That means that we only worry about type (including table, struct, and enum names), enum values, and struct/table fields.
From an implementation this perspective, this moves the translation of the names into the areas that are designed to interface between the
Typesystem of the planner and the Protobuf-based system of Record Layer core. This is then stored in theTypeinformation that is associated with the plan. At the moment, we cheat in a few places and require that the type's fields are always the result of applying the translation function to the display name, or vice versa. However, it allows us to think about allowing custom field names in the future. In a world where we only use Protobuf for storage but have a different mechanism for keeping track of fields in the runtime, we'd need a similar mechanism, though it may take the form of something like a field name to ordinal position mapping, say.This borrows and expands the tests from #3696 and #3706. In particular, it adds additional tests around enums and view and function definitions with non-compliant names, which now work.