Reintroduce type renaming with more resilient handling for unexpected protobuf names #3770
+5,417
−302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… names (FoundationDB#3736)" (FoundationDB#3767) This reverts commit 7f1f4c9.
This adds support for retaining the protobuf names more directly for types and fields. This can happen if the user has created a meta-data proto and used a strategy for naming that differs from the one that would have been generated by our own DML. The basic strategy is to: 1. Continue to always apply the `toProtoUtils` method to produce plausible user-generated names but 1. Retain the original protobuf name in the `Type` information and then use that to get the name used to access data in the field
… that code in the RecordMetadataDeserializer with logic in the Type system
- make sure to convert FieldKeyExpression#fieldName (which is internal) to user-facing name when constructing match candidates. - also, add tests for deeply nested (and repeated) structures with non-pb-compliant field names, and an index.
… can match the indexes in the same circumstances as cases with non-escaped identifiers
📊 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:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reintroduces protobuf type and field renaming. The first version was added in #3696 and #3706 and then taken out by #3726. A second version was introduced with #3736 and then removed by #3767. This adds back a variation based on the data from #3736 but updated to be more resilient to unexpected names in existing meta-data objects.
The issue with #3736 before is that if a type existed in the meta-data that was not correctly escaped (e.g.,
Type__Blahinstead ofType__0Blah), then it would fail to match the type during querying. This was a problem even if the type wasn't actually involved in a query because of how matching worked on theFullUnorderedScanExpression, meaning that any query would fail to plan if any type in the meta-data was so written.This makes things more resilient. We now do a bit more work to associate a type with its original name from the protobuf file if one is provided, recording both the user-visible name and the original storage name. The only places that we now generate new protobuf compliant names is when we construct a
Typeobject. In all other cases, we only go from the storage name to user-visible names.We still do rely on the fact that we can correctly predict the expected user-visible name by running the de-escaping logic. At some point, we may need to have a more complicated mapping, especially if we want to support more arbitrary names. That is left as future work. I could also see us wanting to do a bit more refactoring to better encapsulate this transformation.
The new test modifications made to
valid-identifiers.yamsqlcover those cases by adding new types with names that would not have been generated by any DDL statement, and then validating that (1) those do not disrupt correctly constructed queries and (2) that the problematic types can themselves be queried.In addition, this addresses some shortcomings with the match candidates where
FieldKeyExpressions (which use the internal names) would sometimes be used to generate match candidates which referenced the internal name directly. This fixes that by plugging those gaps. There are additional queries invalid-identifiers.yamsqlthat are designed to cover those matches.