-
-
Notifications
You must be signed in to change notification settings - Fork 237
Hooks example #3303
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
base: main
Are you sure you want to change the base?
Hooks example #3303
Conversation
|
Also, right now tests are going to fail since they don't include the |
zachmu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General idea is sound, but let's avoid introducing more globals if we can help it
|
|
||
| // Global is a variable that contains the interface for calling hooks. By default, this contains no-op hooks as | ||
| // integrators may implement their own hooks. This variable should be overwritten by the integrator. | ||
| var Global Hooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest of the interface is fine, but we should avoid having a global here if at all possible. Instead, put this on the analyzer builder, and then it can be assigned to the session for use in the exec stuff.
| // Hooks is an interface that represents various hooks that are called within a statement's lifecycle. | ||
| type Hooks interface { | ||
| // Table returns hooks related to direct table statements. | ||
| Table() Table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these should probably be plural, returning either a slice of hooks or an iterator over them. We should assume that there is going to be a chain of hooks that integrators set up to run in some order they choose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably just put this in noop.go in the same package as the defn
This is an example of how we could implement a form of dependency injection via hooks. In Doltgres, we'll create our own hooks object that implements the interface, and all throughout the GMS codebase (in the relevant locations), we'd make these calls. We can tailor these calls as well, so it doesn't have to just be
Pre/PostStatement, and we can have ones for intermediate steps as well. This can also be used for many of the free-floating functions that we're reassigning in Doltgres (although it may take a bit of reworking to prevent cycles).Here are two examples:
pg_dependtable).I'm still looking into a better way of removing the singular global variable for defining hooks to ensure that they're available everywhere (engine isn't passed around everywhere, context has cycle issues, etc.), but I'm looking more for feedback on the general approach rather than such specifics.