-
Notifications
You must be signed in to change notification settings - Fork 468
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
adapter: extract an OptimizerCatalog trait #29598
Conversation
This will help with continual tasks, which need to inject an entry for the CT itself before it exists in the catalog, but also it gets us one step closer to pulling the optimizer into its own crate. I also switched over a few more `adapter::optimize::*::Optimizer` structs, but had to leave a few because it gets tricky with `ExprPrepStyle::OneShot` and `eval_unmaterializable_func`. Touches #28899
30fae38
to
83e407e
Compare
This should be ready for review! |
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.
LGTM, but leaving the approval to Gabor.
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.
LGTM!
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.
Clean 👌
TFTRs! |
This will help with continual tasks, which need to inject an entry for
the CT itself before it exists in the catalog, but also it gets us one
step closer to pulling the optimizer into its own crate.
I also switched over a few more
adapter::optimize::*::Optimizer
structs, but had to leave a few because it gets tricky with
ExprPrepStyle::OneShot
andeval_unmaterializable_func
.Touches MaterializeInc/database-issues#8427
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.