This repository was archived by the owner on Dec 9, 2025. It is now read-only.
(feat) Optimizer & Memo Full Logical -> Logical#101
Merged
Conversation
AlSchlo
changed the title
AlSchlo
changed the title
## 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
connortsui20
suggested changes
May 12, 2025
connortsui20
left a comment
connortsui20
left a comment
Member
There was a problem hiding this comment.
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
AlSchlo
pushed a commit
that referenced
this pull request
May 13, 2025
## Summary of changes This PR moves around the modules in the main branch in order to help merge #101 more easily.
connortsui20
approved these changes
May 13, 2025
connortsui20
left a comment
connortsui20
left a comment
Member
There was a problem hiding this comment.
LGTM! I only looked at the things related to the memo but I think this is fine to merge and clean up later.
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
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.