reimplement limit_push_down to remove global-state, enhance optimize and simplify code.
#4276
Conversation
| \n Limit: skip=10, fetch=1000\ | ||
| \n TableScan: test, fetch=1010"; |
There was a problem hiding this comment.
As for limit-tablescan.
I'm not sure whether tablescan can precise limit, so I keep the limit.
If tablescan limit is completely accurate, we can remove this limit for limit-tablescan
There was a problem hiding this comment.
I think we currently don't expect for tablescan to return exactly at most the limit nr of rows AFAIK.
For this to be possible we need to put some more info into the table scan trait, just like we do with statistics.
There was a problem hiding this comment.
Implies that in fact we still need the Limit node even when it has been pushed to the scan as well
There was a problem hiding this comment.
I didn't review the code but I reviewed all the plan changes in the tests and they looked good to me. Thank you @jackwener
| \n Limit: skip=10, fetch=1000\ | ||
| \n TableScan: test, fetch=1010"; |
There was a problem hiding this comment.
Implies that in fact we still need the Limit node even when it has been pushed to the scan as well
| \n Limit: skip=0, fetch=10\ | ||
| \n Limit: skip=10, fetch=10\ | ||
| \n TableScan: test, fetch=20"; | ||
| let expected = "Limit: skip=10, fetch=10\ |
jackwener
force-pushed
the
reimplement_limit_push_down
branch
3 times, most recently
from
303452a
to
ad78bf1
Compare
jackwener
force-pushed
the
reimplement_limit_push_down
branch
from
ad78bf1
to
2fd9813
Compare
| let expected = "Limit: skip=1000, fetch=0\ | ||
| \n TableScan: test, fetch=1000"; | ||
|
|
||
| assert_optimized_plan_eq(&plan, expected) |
There was a problem hiding this comment.
Original test miss case to confirm merge limit, avoid overflow problem.
Add more UT to check them.
jackwener
changed the title
limit_push_downlimit_push_down to remove global-state, enhance optimize and simplify code.
|
@jackwener Thanks |
|
Benchmark runs are scheduled for baseline = afe2333 and contender = 1bcb333. 1bcb333 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?
Part #4267 .
Close #4263
In this PR, I reimplement this rule.
Original implmentation:
global stateAncestorto record the whole tree limit information. It's easy to cause bug and complexRationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?