-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add module structure and unit tests for expression pushdown logical optimizer #20238
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
41073e8 to
3b80d6f
Compare
alamb
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.
Thanks @adriangb -- I started going through some of the tests in this PR and they didn't make a lot of sense to me -- maybe I am missing something
| Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active") | ||
| TableScan: test projection=[id, user] | ||
| "#) | ||
| } |
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.
A minor comment that I think might make this slightly easier to maintain would be to create a single macro./ function
Basically, a function that returns a single String that you then insta review the entire thing
Something like
insta_assert!(do_optimize, @r#"
## Original Plan. < -------- Add heading to separate the plans
Projection: test.id
Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active")
TableScan: test projection=[id, user]
## Plan after Extraction
Projection: test.id
Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active")
TableScan: test projection=[id, user]
## Optimized Plan
assert_optimized!(plan, @r#"
Projection: test.id
Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active")
TableScan: test projection=[id, user]
"#)
You could also do something nice like only print the Plan after extraction and optimized plan if they are different (textually) than the original
insta_assert!(do_optimize, @r#"
## Original Plan
Projection: test.id
Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active")
TableScan: test projection=[id, user]
## Plan after Extraction
(SAME AS ABOVE)
## Optimized Plan
(SAME AS ABOVE)
"#)That would help make it easier to understand what is expected (and would probably cut this diff down by a lot)
| TableScan: test projection=[id, user] | ||
| "#)?; | ||
|
|
||
| // Note: An outer projection is added to preserve the original schema |
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 outer projection is in the initial plan too, right? or is that what you are trying to say with this comment?
| TableScan: test projection=[user] | ||
| "#)?; | ||
|
|
||
| // Projection expressions with MoveTowardsLeafNodes are extracted |
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.
what does this mean "extracted" -- it doesn't seem to have moved anywhere 🤔
|
Sorry @alamb , the comments here are not in sync with the assertions. The comments generally correspond to what we want, the assertions are what the current state with the no-op optimizer rules is. I.e. the comments are unchanged from #20117. I can see now how that's confusing. I can do #20238 (comment) here. Would you rather the comments reflect the current state or the goal state? |
alamb
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.
Sorry @alamb , the comments here are not in sync with the assertions. The comments generally correspond to what we want, the assertions are what the current state with the no-op optimizer rules is. I.e. the comments are unchanged from #20117. I can see now how that's confusing. I can do #20238 (comment) here. Would you rather the comments reflect the current state or the goal state?
Ah -- I see
Maybe we at least add a comment to the datafusion/optimizer/src/extract_leaf_expressions.rs explaining that it is a WIP and that the comments don't yet correspond to the code
3b80d6f to
9c0ae13
Compare
Broken out from #20117 to make diff smaller and easier to review in that PR