Support push down sort after limit

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.

[qianheng-aws]

Description

Support push down sort after limit if there is only limit pushed, since PPL or other database all don't promise sequence for only limit operator. Thus it should be acceptable to transform limit + sort to sort + limit.

However, we should avoid pushing down sort, if there is existing sort before limit. In such case, users intense to retrieve the Top-K values on the first sort fields from our index. The order will be overridden by the second sort if we keep pushing down it. So this PR prevents this case by detecting whether top-k is pushed down already.

Since #4501 has always introduced a limit before sort for join or subsearch, which will block sort push down. This PR will also enhance these scenarios, especially for left join -- both sides will have limit and sort pushed down then. See CalciteExplainIT::testExplainScalarCorrelatedSubqueryInSelect

Related Issues

Resolves#4570

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[songkant-aws]

[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases

[LantaoJin]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

[qianheng-aws]

High level question: At the beginning, I thought you are targeting to push down a sort through sysLimit, so I renamed the PR title. But from the description, it seems a general purpose to push down the sort through limit?

Yes, this a general PR for enhancement not only bug fix.

In the process of finding solution to the issue #4570, I found it hard to only fix that specific issue because syslimit has already been replaced with calcite's logicalsort before optimizing. So we're not able to do anything with the systemlimit operator we added.

But with this general enhancement, the original issue can also been addressed.

[LantaoJin]

can we add a case for below ppl?

source = t | head 100 | stats count() as cnt | sort cnt

The sort cnt must not be pushed down through head 100.

Cannot for following ppl either.

source = t | head 100 | eval rand = rand() | sort rand

[LantaoJin]

Any sort expression evaluated after limit cannot pushed through limit.

[qianheng-aws]

Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:

1. We cannot push down metric sort into agg unless the agg is nullable=false
2. We don't support script push down for sort currently.

[LantaoJin]

could you add these cases in explain IT:

1. It's not the case of pushdown sort agg metrics due to there is no bucket.
2. got it. but better to add a explain IT.