sql/storageparam: break builtins dep on tabledesc #82501
Merged
Conversation
This file contains 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
The TableStorageParamObserver meant that paramparse and transitively builtins depended on tabledesc. This was unfortunate because we want seqexpr is depended on by builtins and we want to use seqexpr in tabledesc, so we have to make sure that builtins does not depend on tabledesc. This commit achieves that goal by splitting out paramparse into three new packages from paramparse: * sql/storageparam: defines the interface for the Setter, contains functions to drive forward the setting and resetting of params, and has some shared functionality. * sql/storageparam/indexstorageparam: implementation of storageparam.Setter for the `descpb.IndexDescriptor`. * sql/storageparam/tablestorageparam: implementation of storageparam.Setter for the `*tabledesc.Mutable`. This allows the `builtins` package to use the `indexstorageparam` package cleanly without depending on `*tabledesc.Mutable`. It also recognizes that lots of utility methods in `paramparse` aren't about `storageparam`s. Release note: None
|
This change is |
otan
approved these changes
Jun 7, 2022
|
TFTR! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The TableStorageParamObserver meant that paramparse and transitively builtins
depended on tabledesc. This was unfortunate because seqexpr is depended
on by builtins and we want to use seqexpr in tabledesc, so we have to make
sure that builtins does not depend on tabledesc.
This commit achieves that goal by splitting out paramparse into three new
packages:
to drive forward the setting and resetting of params, and has some shared
functionality.
for the
descpb.IndexDescriptor.for the
*tabledesc.Mutable.This allows the
builtinspackage to use theindexstorageparampackagecleanly without depending on
*tabledesc.Mutable. It also recognizes thatlots of utility methods in
paramparsearen't aboutstorageparams.Release note: None