Skip to content

Conversation

Asterless
Copy link
Contributor

Summary

Fixed #2613, this PR enhances the @priority_queue by adding an order_by parameter, enabling it to function as both a max-heap (DESC, the default behavior) and a min-heap(ASC).

Key Changes:

  • Added order_by Parameter: The new(), from_array(), and of() functions now accept an optional order_by parameter. This allows users to specify the ordering of elements.

    • @priority_queue.desc (default): The queue operates as a max-heap, with the largest element at the top.
    • @priority_queue.asc: The queue operates as a min-heap, with the smallest element at the top.
  • Updated Internal Logic: The internal functions meld() and merges() have been modified to respect the order_by parameter, ensuring that elements are compared and ordered correctly.

  • Exported asc and desc: To provide a clean public API, asc and desc values of type OrderBy are now exported from the @priority_queue package.

  • Added Tests: New test cases have been added to priority_queue_test.mbt to verify the functionality of both ascending and descending order modes, ensuring that pop and peek return the correct elements in each case.

Example

test "order by" {
  let pq = @priority_queue.new(order_by=@priority_queue.asc)
  pq.push(3)
  pq.push(1)
  pq.push(2)
  assert_eq(pq.pop(), Some(1))
  assert_eq(pq.pop(), Some(2))
  assert_eq(pq.pop(), Some(3))
  assert_eq(pq.pop(), None)
}

@coveralls
Copy link
Collaborator

coveralls commented Sep 22, 2025

Pull Request Test Coverage Report for Build 1305

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 89.509%

Totals Coverage Status
Change from base Build 1304: 0.06%
Covered Lines: 9283
Relevant Lines: 10371

💛 - Coveralls

// Errors

// Types and methods
type OrderBy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this type opaque/abstract?

@quake
Copy link
Contributor

quake commented Sep 23, 2025

In both Rust and Haskell, there is a common idiom called a “newtype” wrapper. The idea is to create a lightweight wrapper around an existing type, without adding runtime overhead, but allowing you to give it different traits/instances.

They are zero-cost abstractions: at runtime, the wrapper disappears.

If moonbit's newtype is zero-cost, I suggest adding a similar newtype in core/cmp instead of adding different sorting parameters to each container that supports sorting.

@hackwaly
Copy link
Contributor

Newtype is too tricky for newcomer. It should be considered as last resort.

@Lampese
Copy link
Collaborator

Lampese commented Sep 24, 2025

It doesn't really matter to me whether or not there is an ORDER BY in actual user usage, but I am more concerned about the API consistency issues it brings.

The current export method is definitely not suitable. Even if this thing is added, it should be placed in a more generic place. I am actually thinking about whether it can be changed to a Boolean value?

The main issue is that our previous approach was similar to the implementation of overloading operators, which involved declaring a new type and overriding its Compare trait to reverse the order. I don't think this is intuitive either.

@peter-jerry-ye
Copy link
Collaborator

I think the problem with newtype is the trade off between code size & performance @bobzhang

The code size will be doubled due to monofy, but the performance would be greater.

@peter-jerry-ye
Copy link
Collaborator

We would follow @quake 's suggestion and provide the @cmp.Reverse.

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.

Require newtype to specify ASC or DESC for priority_queue is inconvenient
6 participants