Skip to content

[fix](function) Reject inconsistent array_sort lambda comparators instead of crashing - #67628

Draft
mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/array-sort-inconsistent-comparator
Draft

[fix](function) Reject inconsistent array_sort lambda comparators instead of crashing#67628
mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/array-sort-inconsistent-comparator

Conversation

@mrhhsg

@mrhhsg mrhhsg commented Sep 7, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Problem Summary:

array_sort hands the user's lambda comparator straight to std::sort.
libstdc++'s introsort relies on the comparator being a deterministic strict
weak ordering: its unguarded partition and unguarded insertion loops walk
past the range as soon as that contract is broken. A comparator such as

SELECT array_sort(
    (x, y) -> IF(x > 100 AND y > 100, -1, IF(x < y, -1, IF(x = y, 0, 1))),
    [1, ..., 10, 101, ..., 160]);

therefore crashes BE with SIGSEGV in ArraySortFunction::execute /
std::__introsort_loop, and the same happens for a non-deterministic
comparator like (x, y) -> IF(random() < 0.5, -1, 1).

A pre-check cannot fix this: detecting every violation before sorting costs
O(n^2) to O(n^3) lambda evaluations, and any sampled check lets some
comparator through to the unguarded sort. So this PR does two things:

  1. Adds bounded_stable_sort, a bottom-up merge sort in which every element
    access is clamped to [first, last) regardless of what the comparator
    answers, and uses it in array_sort. BE can no longer be taken down by a
    comparator. Comparison count is unchanged (O(n log n)), which is what
    dominates because every comparison evaluates the lambda.
  2. After sorting each array, verifies that no adjacent pair of different
    elements satisfies less(next, prev) (n - 1 extra lambda evaluations). If
    the check fails the comparator is not a strict weak ordering and the query
    returns InvalidArgument with a message explaining the contract, instead
    of an unspecified order. A comparator that is only wrong about identical
    elements (the common CASE WHEN x IS NULL THEN -1 WHEN y IS NULL THEN 1 ...
    idiom reports NULL < NULL, and <= instead of <) is tolerated: the
    relative order of identical elements cannot change the sorted output, and
    such comparators already appear in existing tests and queries.

As a side effect the sort is now stable: elements the comparator reports as
equal keep their input order.

Release note

None

Check List (For Author)

  • Test:
    • Unit Test: be/test/util/bounded_stable_sort_test.cpp covers agreement
      with std::stable_sort (including stability), the reported inconsistent
      comparator, always-true / always-false comparators and a random
      comparator, with every index range-checked.
    • Regression test: test_array_sort_lambda_comparator expects the
      strict-weak-ordering error for the reported comparator on literal and
      table input, an always-less comparator and a random comparator; checks
      that <= and NULL < NULL comparators are still accepted; and checks
      large / nullable arrays and stability with consistent comparators.
  • Behavior changed: Yes. An array_sort lambda comparator that is not a
    strict weak ordering now fails the query with InvalidArgument instead of
    crashing BE, and equal elements keep their input order.
  • Does this need documentation: No

https://claude.ai/code/session_016A7UJu7EA7j4NkGz3yjkt6

@mrhhsg

mrhhsg commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

/review

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again.
Workflow run: https://github.com/apache/doris/actions/runs/34136905975

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

…tead of crashing

### What problem does this PR solve?

Issue Number: None

Problem Summary:

`array_sort` hands the user's lambda comparator straight to `std::sort`.
libstdc++'s introsort relies on the comparator being a deterministic strict
weak ordering: its unguarded partition and unguarded insertion loops walk
past the range as soon as that contract is broken. A comparator such as

```sql
SELECT array_sort(
    (x, y) -> IF(x > 100 AND y > 100, -1, IF(x < y, -1, IF(x = y, 0, 1))),
    [1, ..., 10, 101, ..., 160]);
```

therefore crashes BE with SIGSEGV in `ArraySortFunction::execute` /
`std::__introsort_loop`, and the same happens for a non-deterministic
comparator like `(x, y) -> IF(random() < 0.5, -1, 1)`.

A pre-check cannot fix this: detecting every violation before sorting costs
O(n^2) to O(n^3) lambda evaluations, and any sampled check lets some
comparator through to the unguarded sort. So this PR does two things:

1. Adds `bounded_stable_sort`, a bottom-up merge sort in which every element
   access is clamped to `[first, last)` regardless of what the comparator
   answers, and uses it in `array_sort`. BE can no longer be taken down by a
   comparator. Comparison count is unchanged (O(n log n)), which is what
   dominates because every comparison evaluates the lambda.
2. After sorting each array, verifies that no adjacent pair of *different*
   elements satisfies `less(next, prev)` (n - 1 extra lambda evaluations). If
   the check fails the comparator is not a strict weak ordering and the query
   returns `InvalidArgument` with a message explaining the contract, instead
   of an unspecified order. A comparator that is only wrong about identical
   elements (the common `CASE WHEN x IS NULL THEN -1 WHEN y IS NULL THEN 1 ...`
   idiom reports NULL < NULL, and `<=` instead of `<`) is tolerated: the
   relative order of identical elements cannot change the sorted output, and
   such comparators already appear in existing tests and queries.

As a side effect the sort is now stable: elements the comparator reports as
equal keep their input order.

### Release note

None

### Check List (For Author)

- Test:
    - Unit Test: `be/test/util/bounded_stable_sort_test.cpp` covers agreement
      with `std::stable_sort` (including stability), the reported inconsistent
      comparator, always-true / always-false comparators and a random
      comparator, with every index range-checked.
    - Regression test: `test_array_sort_lambda_comparator` expects the
      strict-weak-ordering error for the reported comparator on literal and
      table input, an always-less comparator and a random comparator; checks
      that `<=` and NULL < NULL comparators are still accepted; and checks
      large / nullable arrays and stability with consistent comparators.
- Behavior changed: Yes. An `array_sort` lambda comparator that is not a
  strict weak ordering now fails the query with `InvalidArgument` instead of
  crashing BE, and equal elements keep their input order.
- Does this need documentation: No

Claude-Session: https://claude.ai/code/session_016A7UJu7EA7j4NkGz3yjkt6
@mrhhsg
mrhhsg force-pushed the fix/array-sort-inconsistent-comparator branch from 09280e6 to ca7958c Compare September 7, 2026 16:05
@mrhhsg mrhhsg changed the title [fix](function) Keep array_sort in range when the lambda comparator is inconsistent [fix](function) Reject inconsistent array_sort lambda comparators instead of crashing Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants