Add configuration for eliminating sort in subquery

[irenjj]

Which issue does this PR close?

• Part of [DISCUSSION] Sorts being removed from subqueries #15886

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[kosiew]

Left a few observation notes.

[kosiew]

This should be

enable_eliminate: bool

[kosiew]

enable_eliminate

[kosiew]

Would it be a good idea to add tests for

enable_eliminate_subquery_sort: false

too?

[berkaysynnada]

Hi @irenjj. Thank you working on this, but did you try to set that config to false and run the tests?

I think people should see here how the plans are changed if we do so (perhaps you can open another PR for that -- I don't know if there is an easier way)

As I said in the discussion, I afraid our physical rules are not ready for this. we might either miss some sort eliminations, or possibly eliminate some subquery sorts even if config says opposite. Therefore; I was suggesting enabling this option from lower level to higher. If people still consent this, I couldn't say more but we should add some warnings at least.

[irenjj]

Hi @irenjj. Thank you working on this, but did you try to set that config to false and run the tests?

I think people should see here how the plans are changed if we do so (perhaps you can open another PR for that -- I don't know if there is an easier way)

As I said in the discussion, I afraid our physical rules are not ready for this. we might either miss some sort eliminations, or possibly eliminate some subquery sorts even if config says opposite. Therefore; I was suggesting enabling this option from lower level to higher. If people still consent this, I couldn't say more but we should add some warnings at least.

Yep, I've tried to set config to false and it can forbid eliminating sort in subquery. I'll add a test.

If I approach this case practically, when "order by" clauses are given in subqueries: these are converted into SortExecs at somewhere in the plan. However, in enforce_sorting, we don't track ordering requirements through SortExecs directly (otherwise, we wouldn't be able to eliminate truly necessary SortExecs). Instead, we track the requirement by inserting OutputRequirementExec at the top of the plan, which corresponds to the global ordering - that is the ordering expected when an explicit ORDER BY is given in the outermost query.

If we decide to introduce a new config for this setting, we first need to improve the optimizer phase. Specifically, OutputRequirementExecs could be inserted into intermediate nodes in the plan, meaning the subplan beneath must guarantee to bring the required ordering at that point.
So, TLDR, unless we adapt the current enforce_sorting rule accordingly, we risk losing the ability to eliminate unnecessary SortExecs in some cases.

This is a very good approach. We can implement it later to replace the current approach of adding configuration

[irenjj]

Thanks @kosiew and @berkaysynnada for your review, PTAL👀

[comphead]

Thanks @irenjj
Please include also a test with CTE

with t as (select c1 from sink_table order by c2 nulls last) select * from t

[berkaysynnada]

@irenjj I've set the default value of this config to "false", and auto-complete the slt tests. This is one of the changes:

as I said, even if the logical planner doesn't remove the Sort, it's still eliminated in the physical planning stage. I also thought it's so because of the count(), but if I replace it by another aggregate fn, a SortExec still doesn't emerge in the physical plan (I also consider subquery brings unique results, so remove the inner group by also to be sure). TLDR, we cannot disable subquery sort elimination by disabling it from the logical world only.

[irenjj]

@irenjj I've set the default value of this config to "false", and auto-complete the slt tests. This is one of the changes:

as I said, even if the logical planner doesn't remove the Sort, it's still eliminated in the physical planning stage. I also thought it's so because of the count(), but if I replace it by another aggregate fn, a SortExec still doesn't emerge in the physical plan (I also consider subquery brings unique results, so remove the inner group by also to be sure). TLDR, we cannot disable subquery sort elimination by disabling it from the logical world only.

That's right, we need to process this more elegantly.