Uh oh!
There was an error while loading. Please reload this page.
add module structure and unit tests for expression pushdown logical optimizer - #20238
Conversation
| Filter: mock_leaf(test.user, Utf8("status")) = Utf8("active") | ||
| TableScan: test projection=[id, user] | ||
| "#) | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
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
9c0ae13CompareUh oh!
There was an error while loading. Please reload this page.
…ptimizer (apache#20238) Broken out from apache#20117 to make diff smaller and easier to review in that PR
Followup to apache#20238 bringing in the shared test UDFs from apache#20239
Broken out from #20117 to make diff smaller and easier to review in that PR