reimplment eliminate_limit to remove global-state.
#4324
Conversation
jackwener
changed the title
global-state.eliminate_limit to remove global-state.
| // After remove global-state, we don't record the parent <skip, fetch> | ||
| // So, bottom don't know parent info, so can't eliminate. | ||
| let expected = "Limit: skip=3, fetch=1\ | ||
| \n Sort: test.a, fetch=4\ | ||
| \n Limit: skip=2, fetch=1\ | ||
| \n Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b)]]\ | ||
| \n TableScan: test"; | ||
| assert_optimized_plan_eq_with_pushdown(&plan, expected) |
There was a problem hiding this comment.
Some case need to record information cross plannode.
Exist regression, but it's trivial.
There was a problem hiding this comment.
Removing the global state does look good in some scenarios, but maybe we should keep it where it's necessary?
There was a problem hiding this comment.
basically unnecessary.
- Currently global-state is passed with parameters in a recursive function, can't only some places are reserved
- In fact, it basically does not affect, and other optimizers basically do not consider these corner case.
There was a problem hiding this comment.
I think it would make it easier to see the changes in this PR if the order of the tests was the same (not sure if you reordered the tests on purpose or if that is something related to how github is displaying them)
There was a problem hiding this comment.
Thanks @jackwener for the PR and @waynexia for the review
Sorry for the delay in review (the backlog is substantial these days!)
The code looks great to me, but I don't understand some of the test changes. Can you point out what tests were changed and why?
| // After remove global-state, we don't record the parent <skip, fetch> | ||
| // So, bottom don't know parent info, so can't eliminate. | ||
| let expected = "Limit: skip=3, fetch=1\ | ||
| \n Sort: test.a, fetch=4\ | ||
| \n Limit: skip=2, fetch=1\ | ||
| \n Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b)]]\ | ||
| \n TableScan: test"; | ||
| assert_optimized_plan_eq_with_pushdown(&plan, expected) |
There was a problem hiding this comment.
I think it would make it easier to see the changes in this PR if the order of the tests was the same (not sure if you reordered the tests on purpose or if that is something related to how github is displaying them)
|
Sorry for change UT. It indeed make review hard. I has restored them. original
|
There was a problem hiding this comment.
I reviewed the plan changes in this PR carefully and they looked good to me
Thank you @jackwener
| \n EmptyRelation"; | ||
| assert_optimized_plan_eq(&plan, expected); | ||
| \n Sort: test.a, fetch=3\ | ||
| \n Limit: skip=0, fetch=2\ |
There was a problem hiding this comment.
It took me a while to convince myself this test change was correct
The original plan fetches 2 rows after the aggregate, so the output will only by 2 rows and then does a skip 2, fetch 1 after wards -- so actually this plan will always return no rows but the plan here looks fine and the original actually looks wrong
|
Benchmark runs are scheduled for baseline = dd3f72a and contender = 02da32e. 02da32e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Close #4264.
Part of #4267
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?