Ordered maps #298

MinionJakub · 2025-12-18T11:31:34Z

This is full implementation of Ordered Maps, and it is also a continuation of Pull Request #136 and #288 .

First implementation of Ordered Map and Ordered Set

Small changes to address my mistakes in naming.

First implementation of queues based on implementation of Hood Melville queues from "Purely Functional Data Structures" Chris Okasaki.

Little correction of definitions.

Final touches before merge

… other things to understand what is going on in RedBlackTree

wojpok

I have worked through this PR. Everything is much better then it used to be and hopefully soon everything will be ready. I have some minor issues. My comments apply to entire codebase, I simply didn't want to flood this discussion with repeated comments.

Interface is well polished. I suggest to do some name changes: member to mem and domain to keyList. Other than that, it is all fine.

Documentation requires overhaul. All sentences must start with capital letters and end with periods. Also docstrings need update. We don't support @brief label.

wojpok · 2025-12-18T13:02:26Z

lib/Base/Types.fram


 pub data Either X Y = Left of X | Right of Y

+pub data Ord = Lt | Eq | Gt


Please, add show method for this datatype.

wojpok · 2025-12-18T13:03:42Z

lib/Map.fram

+pub data Map Key = Map of {
+  T
+
+ {## @brief Creates empty map


We don't support @brief label. Simply write this statement and add a period at the end.

wojpok · 2025-12-18T13:05:33Z

lib/Map.fram

+  , empty : {Val} -> T Val
+
+ {## @brief Method to testing whether given map is empty or not
+   # @return True if it's empty false otherwise


Don't add those # symbols in the middle of docstrings.

wojpok · 2025-12-18T13:07:07Z

test/stdlib/stdlib0008_Map.fram


 # Basic tests
 let _ =
-  assert (IntMap.empty {Val=Bool} >.isEmpty);


Tests could be modernized to use the new testing framework.

wojpok · 2025-12-18T13:37:49Z

lib/Map.fram

+  , method foldr = mapFoldr
+  , method toList = toListT
+  , method toValueList = toValueListT
+  , method domain = domainT


This naming is inconsistent. We either stick to domain/codomain or values/keys. I prefer latter honestly

wojpok · 2025-12-18T13:39:48Z

lib/Map.fram

+  , method insertChange = insertChangeT
+  , method remove = removeT 
+  , method removeChange = removeChangeT 
+  , method member = memberT 


Lists already uses mem, so I think it should be used here as well

wojpok · 2025-12-18T13:47:26Z

lib/Map.fram

-each internal node has either two or three children, and all leaves are at the
-same depth. Interestingly, the latter invariant is enforced by types, using
-nested datatypes approach. To do so, we define the following two types.
+# Due to frequent request for understanding this file please


This bit needs to be rewritten. Don't announce any "popular demand", simply mention that the algorithm based on some resarch papers and enumerate them. It will be much more natural

wojpok · 2025-12-18T13:50:56Z

lib/Map.fram

+  | Black =>
+    match child with
+    | Node {value} =>
+      # Must be RED with Leaf children, by black-height invariant.


Some assertion to test this invariant would be nice

wojpok · 2025-12-18T13:52:38Z

lib/Map.fram

+      | Right colorLeftMin leftLeftMin valueLeftMin :: zipperr =>
+        deleteNearLeaf colorLeftMin leftLeftMin
+            (List.append zipperr (Left color valueLeftMin right :: zipper))
+      | _ => Leaf #Fail "postcondition"


Does it really fail or is just misplaced comment?

wojpok · 2025-12-18T13:52:45Z

lib/Map.fram

+    | Left colorRightMin valueRightMin rightRightMin :: zipperr =>
+      deleteNearLeaf colorRightMin rightRightMin
+          (List.append zipperr (Right color left valueRightMin :: zipper))
+    | _ => Leaf #Fail "postcondition"


Copilot

Pull request overview

This PR implements ordered maps using a red-black tree data structure, replacing the previous 2-3 tree implementation. The changes introduce a complete rewrite of the Map module with new API methods and update the comparison interface from separate equal and lt methods to a unified compare method returning an Ord type.

Key changes:

Complete reimplementation of Map using red-black trees instead of 2-3 trees
Introduction of Ord type (Lt, Eq, Gt) for comparisons
API updates including renamed methods (add → insert, mem → member, fold → foldl) and new operations (union, update, mapVal, etc.)

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
lib/Map.fram	Complete rewrite implementing red-black tree-based ordered maps with new API
lib/Base/Types.fram	Added `Ord` data type for three-way comparisons
lib/Base/Int.fram	Added `compare` method to Int type
lib/Base/String.fram	Added `compare` method to String type
lib/Testing.fram	Updated to use new Map API (`add` → `insert`, removed `{key}` syntax)
test/stdlib/stdlib0008_Map.fram	Updated test suite with new API methods and added comprehensive test cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-18T16:09:16Z

test/stdlib/stdlib0008_Map.fram

+# let compareI (x : Int) (y : Int) = if x < y then Lt else if x > y then Gt else Eq  
+# let compareS (x : String) (y : String) = if x < y then Lt else if x > y then Gt else Eq  
+


Commented-out code should be removed. If these comparison functions are intended as examples or documentation, they should be uncommented or moved to proper documentation.

Suggested change

# let compareI (x : Int) (y : Int) = if x < y then Lt else if x > y then Gt else Eq

# let compareS (x : String) (y : String) = if x < y then Lt else if x > y then Gt else Eq

Copilot · 2025-12-18T16:09:17Z

test/stdlib/stdlib0008_Map.fram

+let (Map { module IntMap  }) = Map.make {Key = Int}
+let (Map { module StrMap  }) = Map.make {Key = String}


Inconsistent spacing in record syntax. Consider using consistent spacing: either {Key = Int} for both or {Key=Int} for both to match the style used elsewhere in the file.

Copilot · 2025-12-18T16:09:17Z

lib/Testing.fram

 # Organizes all registered tests into a tree structure
-let Map {module SMap} = Map.make {Key=String}
+
+let Map {module SMap} = Map.make {String}


The Map.make call uses positional syntax {String} instead of the named parameter syntax {Key = String} used elsewhere. This is inconsistent with the API design shown in other files and may be confusing.

Suggested change

let Map {module SMap} = Map.make {String}

let Map {module SMap} = Map.make {Key = String}

Copilot · 2025-12-18T16:09:17Z

lib/Map.fram

-  match ar with
-  | AR_Ok repr       => Tree repr
-  | AR_Split l k v r => Succ (Tree (Node2 l k v r))
+# serachMin finds smallest element in a tree and builds zipper


Corrected spelling of 'serach' to 'search'.

Copilot · 2025-12-18T16:09:18Z

lib/Map.fram

-  match rr with
-  | RR_Ok        node => Succ (Tree node)
-  | RR_Underflow repr => Tree repr
+# serachMax finds largest element in a tree and builds zipper


Corrected spelling of 'serach' to 'search'.

Copilot · 2025-12-18T16:09:18Z

lib/Map.fram

+
+  # The father is red and no nephew is red.
+  # Then father becomes black and brother becomes red
+  # removing therfore the inequality of blackness. 


Corrected spelling of 'therfore' to 'therefore'.

Copilot · 2025-12-18T16:09:18Z

lib/Map.fram

+
+  # The father is red and no nephew is red.
+  # Then father becomes black and brother becomes red
+  # removing therfore the inequality of blackness. 


Corrected spelling of 'therfore' to 'therefore'.

Copilot · 2025-12-18T16:09:18Z

lib/Map.fram

+      | Right colorLeftMin leftLeftMin valueLeftMin :: zipperr =>
+        deleteNearLeaf colorLeftMin leftLeftMin
+            (List.append zipperr (Left color valueLeftMin right :: zipper))
+      | _ => Leaf #Fail "postcondition"


The error handling here is unclear. The comment suggests a postcondition failure, but returning Leaf silently hides this error condition. Consider using a proper error mechanism or at minimum improving the error message to explain what postcondition failed.

Copilot · 2025-12-18T16:09:19Z

lib/Map.fram

+    | Left colorRightMin valueRightMin rightRightMin :: zipperr =>
+      deleteNearLeaf colorRightMin rightRightMin
+          (List.append zipperr (Right color left valueRightMin :: zipper))
+    | _ => Leaf #Fail "postcondition"


The error handling here is unclear. The comment suggests a postcondition failure, but returning Leaf silently hides this error condition. Consider using a proper error mechanism or at minimum improving the error message to explain what postcondition failed.

MinionJakub and others added 30 commits June 19, 2024 08:44

Ordered Map and Ordered Set

7f21e72

First implementation of Ordered Map and Ordered Set

Update OrderedMap.fram

4d6a53a

Small changes to address my mistakes in naming.

Merge branch 'fram-lang:master' into master

c017c3c

Implementation of Queues

ef1d14f

First implementation of queues based on implementation of Hood Melville queues from "Purely Functional Data Structures" Chris Okasaki.

Testing files for Map, Set, Queues.

8bcd905

Update OrderedMap.fram

27ff11e

Little correction of definitions.

Changes to implementation and tests

dae76bc

Adding missing files

e5d206f

Merge branch 'fram-lang:master' into master

ce31a6d

Correcting a test file

5b3cd88

Correcting definitions

eb36254

Changing from [E] in [|E]

274f324

Correcting style and renaming few things

8584322

Merge branch 'fram-lang:master' into master

9aa2f4a

Correcting a lot of things mainly coding style

7bb419d

Changing tests and adding assert to prelude

bb826df

little changes

a3f0b7b

Moving things from Ord and adding fromList and toList to Queue

c44f26c

onError methods

b9eb957

correcting test for names changes

88ee81d

little change

6727f66

Merge branch 'fram-lang:master' into master

e462f83

Small changes

9ade649

Adding comments

2904ee7

Merge branch 'fram-lang:master' into master

9dc0c0c

Some (hopefully) final changes

7469061

Replacing '\t' with ' '

c4675a9

Final touches before merge by Foxinio

cd894b1

Final touches before merge

Fix file permissions

8f31eb6

Merge branch 'fram-lang:master' into master

ec465d9

MinionJakub and others added 9 commits April 4, 2025 10:52

Correction due to change in syntax

394a919

Merge branch 'fram-lang:master' into master

dd86d99

Correcting small mistakes and adding some longly requested papers and…

5450ed1

… other things to understand what is going on in RedBlackTree

Merge branch 'fram-lang:master' into master

f9ccbf0

Merge branch 'fram-lang:master' into master

b447f1e

Changing to incremental changes - for now queues + red-black tree

2549f93

Red Black Tree Comments

2e8cf76

Merge branch 'master' into RedBlackTrees

bdb2dde

Full implementation of Maps with tests

df1aee8

MinionJakub force-pushed the OrderedMaps branch from fb83300 to 6acd9b2 Compare December 18, 2025 11:45

MinionJakub added the stdlib Work on standard library label Dec 18, 2025

MinionJakub force-pushed the OrderedMaps branch from 6acd9b2 to 4285976 Compare December 18, 2025 12:44

Merge branch 'fram-lang:master' into OrderedMaps

be71b7a

MinionJakub force-pushed the OrderedMaps branch from 4285976 to be71b7a Compare December 18, 2025 12:54

wojpok suggested changes Dec 18, 2025

View reviewed changes

ppolesiuk requested a review from Copilot December 18, 2025 16:04

Copilot AI reviewed Dec 18, 2025

View reviewed changes


		pub data Either X Y = Left of X \| Right of Y

		pub data Ord = Lt \| Eq \| Gt

		# let compareI (x : Int) (y : Int) = if x < y then Lt else if x > y then Gt else Eq
		# let compareS (x : String) (y : String) = if x < y then Lt else if x > y then Gt else Eq

		let (Map { module IntMap }) = Map.make {Key = Int}
		let (Map { module StrMap }) = Map.make {Key = String}

	let Map {module SMap} = Map.make {String}
	let Map {module SMap} = Map.make {Key = String}

Ordered maps #298

Are you sure you want to change the base?

Ordered maps #298

Uh oh!

Conversation

MinionJakub commented Dec 18, 2025

Uh oh!

wojpok left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wojpok left a comment •

edited

Loading