-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(df-repr): add back join order enumeration #204
Conversation
Signed-off-by: Alex Chi <[email protected]>
c24335b
to
0e89d99
Compare
Signed-off-by: Alex Chi <[email protected]>
(physical join not supported for now, and I don't think it's necessary to support it) |
@@ -369,6 +369,10 @@ impl<T: RelNodeTyp> CascadesOptimizer<T> { | |||
.map(|x| x.cost.0[0]) | |||
.unwrap_or(0.0) | |||
} | |||
|
|||
pub fn memo(&self) -> &Memo<T> { |
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.
Exposing the memo table publicly strikes me as a bit scary. I am worried users of the library might try to manipulate the memo table manually.
I'm hoping access is read-only (looks like it is?)
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.
yes it's read only 🤪
// logical_join_orders.iter().map(|x| x.to_string()).join("\n"), | ||
// )); | ||
// } | ||
let join_orders = optimizer |
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.
Is this optional or always calculated?
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.
always calculated, as before, unless we can find a better way of passing options through datafusion SQL...
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.
that's why I didn't close the issue, this is mentioned in the issue
@@ -0,0 +1,186 @@ | |||
//! Memo table extensions |
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.
definitely much cleaner than in the bridge.
- I think we'll need to be careful to document it so that it isn't misused by users
- I think the persistent memo table stuff might actually make this extension system unnecessary, since we can read the database to see which join orders were enumerated.
For now though, definitely an improvement!
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.
the join information is not directly persisted in the database? we still need to go through this depth-first search algorithm to reconstruct such info.
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.
Right, but then we can do it after the query runs instead of checking the memo table "in vivo"
enable auto-merge... |
Signed-off-by: Alex Chi <[email protected]>
ref #194
after the memo table refactor, adding back a more efficient join order enumeration implementation.