Fix internal error in sort when hitting memory limit #15692

DerGut · 2025-04-12T13:31:20Z

Which issue does this PR close?

Closes Internal error in ExternalSorter when running with memory limit #15675

Rationale for this change

I noticed an internal error in the sort implementation. This PR is exposing it as a proper user-facing error instead.

What changes are included in this PR?

If a user hits this condition, they now receive a DataFusionError:ResourcesExhausted instead of DataFusionError::Internal.

Are these changes tested?

Yes.

Are there any user-facing changes?

In away, the code now exposes a different error to the user facing API. However, since the old error was internal, this was a bug and I don't expect any documentation changes are necessary.

squash! Fix clone_on_ref_ptr Clippy warning squash! Move test import to test module squash! Be more explicit about triggering config values

datafusion/physical-plan/src/sorts/sort.rs

DerGut · 2025-04-12T17:12:04Z

datafusion/physical-plan/src/sorts/sort.rs

+        let result = self.reservation.try_grow(size);
+        if result.is_err() {
+            if self.in_mem_batches.is_empty() {
+                return result;


@2010YOUY01 since you commented on the issue #15675 (comment) with suggestions:

I think the fix (for a more user-friendly error message) would be

When sort_spill_reservation_byte reservation failed, the error message should include considering set the memory limit larger than this reservation memory size

For the code path triggered this internal error, return an ExecutionError instead with a similar error message

This code now emits a DataFusionError::ResourcesExhausted instead of the suggested DataFusionError::Execution. I think this makes sense, given the descriptions of the errors.

E.g. Execution error doesn't seem to apply:

This error is returned when an error happens during execution due to a malformed input.

but ResourcesExhausted does:

This error is thrown when a consumer cannot acquire additional memory

I still like the idea of suggesting to bump the memory limit in the error message.

However, since the code simply bubbles up the error emitted by the memory pool's insufficient_capacity_err macro, we would either need to map it, or change the macro's message.

I don't think mapping makes sense here because the error is generally used for reservation-related issues - I don't think this is a special case.
Changing the error message would probably require a separate discussion.

It makes sense that we should preserve the raw resource error. In this case, ContextError comes in handy:

datafusion/datafusion/common/src/error.rs

Line 132 in 0b01fdf

Context(String, Box<DataFusionError>),

It can add extra notes to the original error message, to make it more understandable.

Here, the extra note can be like "When the memory limit is reached and there are no buffered batches to spill, external sort cannot continue. Consider setting a higher memory limit."

This is great! Thanks a lot for the reference 🙏

I added some context with d4e654c

I dropped the detail about buffered batches because I figured that it's probably not relevant to the user. Regardless of whether in-memory batches could be spilled or not, the actionable info is to increase the memory limit. This also allowed me to reuse the same context in a14e585 (and 0ee1e8e for completeness). This means all failing memory reservations in the external sorter are wrapped with context. Let me know if you don't think these are necessary.

I also added a suggestion to decrease sort_spill_reservation_bytes instead. But I'm not convinced this is good advice to the user. I can leave it as a comment instead 🤷

2010YOUY01

Thank you for this rapid fix! I left some comments, and would love to hear your take.

datafusion/physical-plan/src/sorts/sort.rs

2010YOUY01 · 2025-04-13T03:17:13Z

datafusion/physical-plan/src/sorts/sort.rs

+        let result = self.reservation.try_grow(size);
+        if result.is_err() {
+            if self.in_mem_batches.is_empty() {
+                return result;


It makes sense that we should preserve the raw resource error. In this case, ContextError comes in handy:

datafusion/datafusion/common/src/error.rs

Line 132 in 0b01fdf

Context(String, Box<DataFusionError>),

It can add extra notes to the original error message, to make it more understandable.

Here, the extra note can be like "When the memory limit is reached and there are no buffered batches to spill, external sort cannot continue. Consider setting a higher memory limit."

datafusion/physical-plan/src/sorts/sort.rs

Signed-off-by: DerGut <[email protected]> squash! Use internal_err macro

Signed-off-by: DerGut <[email protected]>

rluvaton · 2025-04-13T23:13:03Z

datafusion/physical-plan/src/sorts/sort.rs

+            DataFusionError::ResourcesExhausted(_) => e.context(format!(
+                "Not enough memory to continue external sort. \
+                    Consider increasing the memory limit, or decreasing sort_spill_reservation_bytes"
+            )),


If I match on the result of this function will it still be resources exhausted or different type, if different type this can break usages that depend on that, no?

That's a good point! It will be a different type (or enum variant), that's also why the test is currently failing (see https://github.com/apache/datafusion/pull/15692/files#r2041251481)

This would actually be a good argument for adding a function to DataFusionError that allows you to safely compare against any of its nested errors (I'm thinking something similar to Go's errors.Is).

Without such an API, it would never be safe to just wrap an error with context 🤔

I agree it's better to have a utility to assert an error's root cause, like
Context(Context(ResourceExhausted, ...'s root cause is ResourceExhanusted, however, I think this is not required for this PR.

Works for me :) CI is passing again and I'l create a separate issue 👍

Created this: #15713

DerGut · 2025-04-14T00:01:56Z

datafusion/physical-plan/src/sorts/sort.rs

+
+        let err = result.unwrap_err();
+        assert!(
+            matches!(err, DataFusionError::ResourcesExhausted(_)),


Note, this is now failing because it gets a DataFusionError::Context instead. I'm still pondering whether to ~~unwrap~~ unnest the error here, or add something like an DataFusionError::is(&self, target: &DataFusionError) -> bool to match the nested error.
I don't think matching against Context instead would be interesting here. I'd rather test the cause of the error, less so whether it has added context or not.

Made it pass by doing the unnesting in-line for now d2a0765. I'll create a separate issue to discuss a potential API extension of DataFusionError.

2010YOUY01 · 2025-04-14T04:09:05Z

Thank you, it looks good to me 🙏🏼
Let's make the CI pass, I think we can change the assertion type for simplicity here, and do a separate PR for this utility DataFusionError::is(&self, target: &DataFusionError) -> bool if you prefer.

Signed-off-by: Jannik Steinmann <[email protected]>

DerGut force-pushed the main branch 2 times, most recently from 50f7266 to ae0b6a5 Compare April 12, 2025 15:47

DerGut added 3 commits April 12, 2025 17:48

Test sort batch reservation with too little memory

9623574

squash! Fix clone_on_ref_ptr Clippy warning squash! Move test import to test module squash! Be more explicit about triggering config values

Fix internal error

37acdae

Handle no in-mem batches in dedicated function

99a1944

DerGut force-pushed the main branch from ae0b6a5 to 9623574 Compare April 12, 2025 15:49

DerGut commented Apr 12, 2025

View reviewed changes

datafusion/physical-plan/src/sorts/sort.rs Show resolved Hide resolved

DerGut commented Apr 12, 2025

View reviewed changes

DerGut marked this pull request as ready for review April 12, 2025 17:12

2010YOUY01 reviewed Apr 13, 2025

View reviewed changes

Error if sort_and_spill_in_mem_batches is called without in-mem batches

c3e5388

Signed-off-by: DerGut <[email protected]> squash! Use internal_err macro

DerGut force-pushed the main branch from a5c6143 to c3e5388 Compare April 13, 2025 21:07

DerGut added 4 commits April 13, 2025 23:29

Make reserve_memory_for_batch explicit about spilling

ba24781

Signed-off-by: DerGut <[email protected]>

Add context to OOM errors

d4e654c

Signed-off-by: DerGut <[email protected]>

Add OOM context for merge reservation

a14e585

Add OOM context in in-mem sort stream

0ee1e8e

Signed-off-by: DerGut <[email protected]>

rluvaton reviewed Apr 13, 2025

View reviewed changes

DerGut commented Apr 14, 2025

View reviewed changes

DerGut added 3 commits April 14, 2025 12:23

Unnest context error in-line

d2a0765

Signed-off-by: Jannik Steinmann <[email protected]>

squash! Remove unnecessary use of format! macro

9569542

Use DataFusionError::find_root() in test

0e65dca

DerGut mentioned this pull request Apr 14, 2025

API to match against any error in chain #15713

Open

2010YOUY01 approved these changes Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix internal error in sort when hitting memory limit #15692

Fix internal error in sort when hitting memory limit #15692

DerGut commented Apr 12, 2025 •

edited by alamb

Loading

DerGut Apr 12, 2025

2010YOUY01 Apr 13, 2025

DerGut Apr 13, 2025 •

edited

Loading

2010YOUY01 left a comment

2010YOUY01 Apr 13, 2025

rluvaton Apr 13, 2025

DerGut Apr 14, 2025 •

edited

Loading

DerGut Apr 14, 2025 •

edited

Loading

2010YOUY01 Apr 14, 2025

DerGut Apr 14, 2025

DerGut Apr 14, 2025

DerGut Apr 14, 2025 •

edited

Loading

DerGut Apr 14, 2025

2010YOUY01 commented Apr 14, 2025

Fix internal error in sort when hitting memory limit #15692

Are you sure you want to change the base?

Fix internal error in sort when hitting memory limit #15692

Conversation

DerGut commented Apr 12, 2025 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerGut Apr 13, 2025 • edited Loading

Choose a reason for hiding this comment

2010YOUY01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerGut Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

DerGut Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerGut Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2010YOUY01 commented Apr 14, 2025

DerGut commented Apr 12, 2025 •

edited by alamb

Loading

DerGut Apr 13, 2025 •

edited

Loading

DerGut Apr 14, 2025 •

edited

Loading

DerGut Apr 14, 2025 •

edited

Loading

DerGut Apr 14, 2025 •

edited

Loading