-
Notifications
You must be signed in to change notification settings - Fork 4
(feat) Optimizer & Memo Full Logical -> Logical #101
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
## Summary of changes This PR moves the memo table from the previous WIP branch over to this fresh branch. It is not completely cleaned up yet. I am considering removing `memory.rs` for now while I clean it up. Also, I cleaned up the bridges to not use glob imports
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.
Very nice job! I like the design here and the fact that generally different ideas are separated along different methods and types. I think the new group ID for newly merged groups is also generally a good idea now - before when we were trying to think about parallelism it wasn't ideal but since that is out the window we don't really care about that anymore.
I suggested a few things, but the only thing that I guess is worth discussing is the error handling on the memo table. Basically, what should happen if something goes wrong? See my comments above.
## Problem Because the memo table implementations are likely going to have very different error conditions, we would ideally like the implementors of the memo table traits to define their errors instead of us. For example, a persistent memo table on disk will want to return I/O errors, but an in-memory memo table should be infallible. A memo table as a service over the network might have completely different errors, and a parallel memo table could have different behavior as well. ## Summary of changes This PR modifies the `memo` module to use a new `MemoBase` trait that allows the implementors to define an associated error type. ```rust /// Base trait defining a shared implemention-defined error type for all memo-related traits. pub trait MemoBase { /// The error type used throughout all memo-related traits. type MemoError: Debug; } ``` The reason we need this is because the memo traits have been effectively divided into 3 subtraits, and we cannot specify 1 associated type for 3 different traits. --- Note that the error type for the in-memory memo table is `Infallible`. ```rust /// The never type. Used for ensuring that the in-memory memo table never raises an error. #[derive(Debug)] pub enum Infallible {} /// This defines the error type for the in-memory implementation of the memo table. impl MemoBase for MemoryMemo { /// In-memory operations cannot fail. type MemoError = Infallible; } ``` This could change in the future, but at least for now it is not really clear what a user of the in-memory table would do if it encountered a broken invariant (a group does not exist, for example). So this PR simply makes all of those error cases a panic / crash. This can easily be changed in the future. # TODO - [x] Figure out how to error handle in the optimizer code - [x] Refactor how the `optimizer` module handles errors with `OptimizerError`, since `MemoError` used to be the only variant - [x] Clean up lints
## Summary of changes This PR moves around the modules in the main branch in order to help merge #101 more easily.
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! I only looked at the things related to the memo but I think this is fine to merge and clean up later.
Implements the in-memory memo for logical -> logical, as well as the optimizer task graph, and the support of
*
stored expressions in the compiler.All features are tested accordingly, except the optimizer itself which will be postponed in a future PR.