Minor: use the minimum fetch

[xudong963]

Which issue does this PR close?

Closes #14192 (comment)

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[berkaysynnada]

Thanks @xudong963. Can you also add a small unit test?

[xudong963]

Thanks @xudong963. Can you also add a small unit test?

Thanks, I'll do it later

[xudong963]

It's difficult for me to construct such ut, I didn't construct a test that will go to the condition

datafusion/datafusion/core/src/physical_optimizer/sort_pushdown.rs

Lines 106 to 112 in 5edb276

	if !satisfy_parent {
	// Make sure this `SortExec` satisfies parent requirements:
	let sort_reqs = requirements.data.ordering_requirement.unwrap_or_default();
	// It's possible current plan (`SortExec`) has a fetch value.
	let fetch = requirements.data.fetch.or(sort_fetch);
	requirements = requirements.children.swap_remove(0);
	requirements = add_sort_above(requirements, sort_reqs, fetch);

.

Do you have any ideas? @berkaysynnada

[alamb]

It's difficult for me to construct such ut, I didn't construct a test that will go to the condition

I don't think it is possible today to hit this code (as the requirements will contain the sort correctly)

Thus I think it would be fine to merge this as is in my opinion, or perhaps you could add a check that the requirement_fetch is <= the sort_fetch if present

[berkaysynnada]

It's difficult for me to construct such ut, I didn't construct a test that will go to the condition

datafusion/datafusion/core/src/physical_optimizer/sort_pushdown.rs

Lines 106 to 112 in 5edb276

	if !satisfy_parent {
	// Make sure this `SortExec` satisfies parent requirements:
	let sort_reqs = requirements.data.ordering_requirement.unwrap_or_default();
	// It's possible current plan (`SortExec`) has a fetch value.
	let fetch = requirements.data.fetch.or(sort_fetch);
	requirements = requirements.children.swap_remove(0);
	requirements = add_sort_above(requirements, sort_reqs, fetch);

.
Do you have any ideas? @berkaysynnada

It doesn't seem possible without explicitly setting it (and that would not have much meaning). Let's merge this once you've resolved the conflicts.

[xudong963]

Thanks @alamb @berkaysynnada