-
Notifications
You must be signed in to change notification settings - Fork 117
Revert "Reintroduce translation of identifiers to Protobuf compliant names (#3736)" #3767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…names (FoundationDB#3736)" This reverts commit 4873b51 (PR FoundationDB#3736) in response to some issues seen with how this may result in the planner failing to produce a query for some queries. The exact circumstances required for this are still under investigation, so rather than fix the bug, this reverts the renaming change.
📊 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. Dropped QueriesThe following queries with metrics were removed:
This list has been truncated. There are 30 remaining dropped queries. The reviewer should double check that these queries were removed intentionally to avoid a loss of coverage. |
|
The test gaps are for methods that I modified as part of the PR. There are tests of what the new behavior is, but I can't take out the old behavior cleanly and introduce new tests. I'm proposing we bypass that check and merge anyway given the situation |
…mpliant names (FoundationDB#3736)" (FoundationDB#3767)" This reverts commit 7f1f4c9.
… names (FoundationDB#3736)" (FoundationDB#3767) This reverts commit 7f1f4c9.
… protobuf names (#3770) 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__Blah` instead of `Type__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 the `FullUnorderedScanExpression`, 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 `Type` object. 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.yamsql` cover 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 `FieldKeyExpression`s (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 in `valid-identifiers.yamsql` that are designed to cover those matches. --------- Co-authored-by: Youssef Hatem <y_hatem@apple.com>
This reverts commit 4873b51 (PR #3736) in response to some issues seen with how this may result in the planner failing to produce a query for some queries. The exact circumstances required for this are still under investigation, so rather than fix the bug, this reverts the renaming change.