Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data-backed map for Plutus Tx #5701

Closed
wants to merge 3 commits into from
Closed

Data-backed map for Plutus Tx #5701

wants to merge 3 commits into from

Conversation

zliu41
Copy link
Member

@zliu41 zliu41 commented Jan 4, 2024

To avoid very large PRs I'd like to merge this part first.

This PR contains:

  • A Data-backed Map type in PlutusTx.DataMap, and some tests.
  • A new Value type backed by the new Map type in PlutusLedgerApi.Data.V1.Value.
  • Updated the Marlowe validator to use the new Map type in State.

I need to write more tests, but otherwise this is ready for review.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

Ran out of time, will continue reviewing tomorrow.

Comment on lines +329 to +335
-- SCP-5126: The return value of this function differs from
-- Isabelle semantics in that it returns the least-recently
-- added account-token combination rather than the first
-- lexicographically ordered one. Also, the sequence
-- `Map.fromList . tail . Map.toList` preserves the
-- invariants of order and non-duplication.
let (((accId, token), balance), rest) = Map.unsafeUncons accounts
Copy link
Contributor

@effectfully effectfully Jan 5, 2024

Choose a reason for hiding this comment

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

Is the comment still true? At the very least the Map.fromList . tail . Map.toList point seems obsolete. Someone from the Marlowe team should look at this, I think.

&& traceIfFalse ("ea" <> tag) (noDuplicates accounts)
&& traceIfFalse ("ec" <> tag) (noDuplicates choices)
&& traceIfFalse ("eb" <> tag) (noDuplicates boundValues)
&& traceIfFalse ("ea" <> tag) (Map.noDuplicateKeys accounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised the Map module provides such a function, but OK.

Comment on lines +76 to +77
-- succeeding equality.
-- check if the non-empty list only contains zero values.
Copy link
Contributor

Choose a reason for hiding this comment

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

That was probably my typo, but please remove that dot and the newline after "equality", it's a single sentence.

, Maybe PlutusTx.BuiltinByteString
)
)
map1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

A short description of what we're testing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think it would be appropriate to do some "model" property-based tests where we test that this map behaves like a Haskell map and/or AssocMap under various series of operations.

have efficient `P.toBuiltinData` and `P.unsafeFromBuiltinData` operations (e.g., they
are primitive types or types defined using @asData@).
-}
newtype Map k a = Map P.BuiltinData
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment on whether there are any invariants such as orderness? (Stipulating the lack of orderness would also be helpful)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the invariant is just no-duplicates, and not orderedness. I wonder if having it be ordered would help... then it wouldn't be a drop-in replacement for AssocMap since you would need Ord, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the notion of invariant a bit tenuous here? It would be easy to construct a data object that looks like a map but doesn't satisfy the invariants and stick it in a datum or something. You might even do that by accident if you're using some external tool to produce data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the notion of invariant a bit tenuous here?

Either way, I just wanna know if there are any or not.

It would be easy to construct a data object that looks like a map but doesn't satisfy the invariants and stick it in a datum or something. You might even do that by accident if you're using some external tool to produce data.

A Map isn't necessarily taken verbatim from a datum. It may be produced by the validator itself or come from a datum but be properly sanitized, so I don't think having invariants is out of question. But maybe I misunderstand the intended use cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the invariant is just no-duplicates

It doesn't guarantee no-duplicates. This is really an association list, rather than a map, and should probably be named so.

Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

I've not looked at the whole thing yet: I'll come back and finish next week.

{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE ViewPatterns #-}
{-# OPTIONS_GHC -fplugin-opt PlutusTx.Plugin:context-level=0 #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually do anything? I experimented with this when I was trying to improve the error messages for ranges like [1.10] and it didn't seem to make any difference. I meant to go back and look at it more carefully but haven't done so yet.

{-# LANGUAGE NoImplicitPrelude #-}
{-# LANGUAGE ViewPatterns #-}

module PlutusLedgerApi.Data.V1.Value where
Copy link
Contributor

Choose a reason for hiding this comment

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

This is replicating (nearly) a lot of stuff in PlutusLedgerApi.V1.Value. Is the idea that you're going to replace that with this in a later PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I feel like maybe we should talk about how we're going to do the API. Duplicating it all is just going to suck, but I don't know what alternative we have if the new version isn't a strict improvement on the old one (which it doesn't seem like it is...).

{-# LANGUAGE TupleSections #-}
{-# LANGUAGE ViewPatterns #-}

module PlutusTx.DataMap where
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to say what you're exporting here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will add explicit exports.

checks on keys can be faster due to `P.equalsData`.
* Many operations involve converting the keys and/or values to/from `P.BuiltinData`.

Therefore this map implementation is likely a better choice than @PlutusTx.AssocMap.Map@
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, are you going to keep both of them and let the user choose? If you're going to do that you could maybe have property tests to check that both versions do the same thing. Also in that case it would be worth mentioning it in PlutusTx.AssocMap.Map.

xs
Nothing
( \hd tl ->
let k' = BI.fst hd
Copy link
Contributor

Choose a reason for hiding this comment

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

The alignment seems a bit odd here (and elsewhere in this file).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fourmolu - it would take me forever to hand-format this module!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fourmolu - it would take me forever to hand-format this module!

Ahhhh. I've never tried using fourmolu. I just let stylish-haskell do its thing, and it does something totally different!

go xs =
P.matchList
xs
(BI.mkCons (BI.mkPairData k a) nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that if the key's not already in the map then this always inserts the new entry at the end and has to duplicate all of the original list when it does BI.mkCons hd (go tl) a few lines below here. Could we somehow just cons the new entry onto the front in that case? That would presumably be more memory-efficient.

Later: I thought you could avoid duplicating the original input by binding it to a variable at the start and consing onto that if you arrive at the end without finding the key, but you do have to copy the front of the list if the key actually is present and you can't tell that it's safe not to do that until you get to the end. Hmmm.

Later still: I suppose you could scan the whole list first to see if your key's already there and do the cheap cons if it isn't, but then you're paying for a possible memory saving with the CPU cost of an extra traversal.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to repeat all the equalsData checks though, so the first pass has to remember where to insert the new pair if the key exists in the map.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose you could scan the whole list first to see if your key's already there and do the cheap cons if it isn't, but then you're paying for a possible memory saving with the CPU cost of an extra traversal.

I doubt doing two traversals for insertion is the more efficient implementation in any programming language (do people do that in any language?), and thus isn't the right thing to do in PLC either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose in general you'd use some structure that lets you do efficient insertion, but trying to do something like that in PLC might have too much overhead. The trouble is that we're trying to maintain an invariant of every key only occurring once, and that's maybe a bad fit with lists. I suppose we could do some experiments to see what the space/time tradeoff is like if we do two traversals. Maybe the space usage isn't too bad though: you do have to copy all of the nodes if the thing you're inserting isn't in the map, but it's a shallow copy where you just need to allocate new cons-cells that contain pointers to the contents without copying the contents themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the space usage isn't too bad

... although if you create a map with n entries by calling insert n times you'll allocate n(n+1)/2 cons cells in total.

let k' = BI.fst hd
in if P.equalsData k k'
then tl
else BI.mkCons hd (go tl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I think this duplicates the entire list if you delete a non-existent entry.

. toBuiltin
. PlutusTx.Prelude.map (\(k, a) -> (P.toBuiltinData k, P.toBuiltinData a))

{-# INLINEABLE uncons #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you ever want to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a bit weird, since it depends on the ordering

Copy link
Member Author

Choose a reason for hiding this comment

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

The Marlowe validator needs unsafeUncons.

This data structure is really an association list rather than a map, and should probably be named so. For an association list, it makes a lot more sense to have an uncons operation.

acc
(\hd -> go (BI.mkCons hd acc))

-- | Combine two 'Map's with the given combination function.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to have a comment saying what the semantics are if you've got an overlap.


{-# INLINEABLE union #-}

-- | Combine two 'Map's.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this is doing, but it's bedtime so I'll submit what I've said so far and come back for another look next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's an inlined and de-abstracted version of the one in AssocMap. I'm not sure why it's not a more direct port.

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I would like to know why it's not always faster!


import PlutusTx.Builtins qualified as PlutusTx
import PlutusTx.Code
import PlutusTx.DataMap qualified as Map
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we implement these tests with AssocMap also to check the difference?

null = P.null . toBuiltinList

{-# INLINEABLE unsafeFromList #-}
unsafeFromList :: (P.ToData k, P.ToData a) => [(k, a)] -> Map k a
Copy link
Contributor

Choose a reason for hiding this comment

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

lots of this stuff needs haddock, explaining what things do and in this case why it's unsafe

. toBuiltin
. PlutusTx.Prelude.map (\(k, a) -> (P.toBuiltinData k, P.toBuiltinData a))

{-# INLINEABLE uncons #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a bit weird, since it depends on the ordering

(k, a) = P.pairToPair hd

{-# INLINEABLE noDuplicateKeys #-}
noDuplicateKeys :: forall k a. Map k a -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

this is quadratic, worth saying

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, if we call this an association list, then this would be a lot more obvious.

)

{-# INLINEABLE all #-}
all :: forall k a. (P.UnsafeFromData a) => (a -> Bool) -> Map k a -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

why not define a fold and use that for a bunch of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

generally it seems like you've avoided defining any higher-order functions and using them, which seems like overkill to me, and makes this much harder to review...

Copy link
Contributor

@effectfully effectfully Jan 5, 2024

Choose a reason for hiding this comment

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

why not define a fold and use that for a bunch of these?

That would make it trickier to make sure that things short-circuit properly, although GHC inlining probably is more or less reliable in that case, although relying on GHC inlining is always dangerous.

Copy link
Member Author

Choose a reason for hiding this comment

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

why not define a fold

Then fold will have to be inlined and beta-reduced, but there's no guarantee, even if it has the INLINE pragma. Recall that the INLINE pragma only makes it inline, not beta-reduce.

The code duplication shouldn't be a big deal due to CSE.


{-# INLINEABLE union #-}

-- | Combine two 'Map's.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's an inlined and de-abstracted version of the one in AssocMap. I'm not sure why it's not a more direct port.

{-# LANGUAGE NoImplicitPrelude #-}
{-# LANGUAGE ViewPatterns #-}

module PlutusLedgerApi.Data.V1.Value where
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I feel like maybe we should talk about how we're going to do the API. Duplicating it all is just going to suck, but I don't know what alternative we have if the new version isn't a strict improvement on the old one (which it doesn't seem like it is...).

, Maybe PlutusTx.BuiltinByteString
)
)
map1 =
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think it would be appropriate to do some "model" property-based tests where we test that this map behaves like a Haskell map and/or AssocMap under various series of operations.

have efficient `P.toBuiltinData` and `P.unsafeFromBuiltinData` operations (e.g., they
are primitive types or types defined using @asData@).
-}
newtype Map k a = Map P.BuiltinData
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the invariant is just no-duplicates, and not orderedness. I wonder if having it be ordered would help... then it wouldn't be a drop-in replacement for AssocMap since you would need Ord, I guess.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

As others said, tests and docs are missing, otherwise seems good, although I do share Kenneth's performance concerns.

UPD: oh yeah, forgot to say, I didn't review the union functions.

toBuiltinData (Map d) = d

instance P.FromData (Map k a) where
fromBuiltinData = Just . Map
Copy link
Contributor

Choose a reason for hiding this comment

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

So no invariants it seems then.

Nothing -> Nothing

{-# INLINEABLE lookup' #-}
lookup' ::
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of the name, but OK.

( \hd tl ->
let k' = BI.fst hd
in if P.equalsData k k'
then Just (BI.snd hd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling both fst and snd manually is gonna become obsolete eventually when we have pattern matching builtins (either way), so we should probably add matchPair alongside matchList and use the former here (given that you already use the latter it'll be more consistent anyway).

Nothing
( \hd tl ->
let k' = BI.fst hd
in if P.equalsData k k'
Copy link
Contributor

Choose a reason for hiding this comment

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

So we're going over a list and checking equality of tree-like structures... I wonder if those are ever going to have a lot of common prefixes. If so, a fancy form of trie would perform much better.

go xs =
P.matchList
xs
(BI.mkCons (BI.mkPairData k a) nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to repeat all the equalsData checks though, so the first pass has to remember where to insert the new pair if the key exists in the map.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I'm missing it, is there any way to safely convert a list to a map?

Copy link
Member Author

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

Responded to some comments.

{-# LANGUAGE TupleSections #-}
{-# LANGUAGE ViewPatterns #-}

module PlutusTx.DataMap where
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will add explicit exports.

have efficient `P.toBuiltinData` and `P.unsafeFromBuiltinData` operations (e.g., they
are primitive types or types defined using @asData@).
-}
newtype Map k a = Map P.BuiltinData
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the invariant is just no-duplicates

It doesn't guarantee no-duplicates. This is really an association list, rather than a map, and should probably be named so.

xs
Nothing
( \hd tl ->
let k' = BI.fst hd
Copy link
Member Author

Choose a reason for hiding this comment

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

It's fourmolu - it would take me forever to hand-format this module!

go xs =
P.matchList
xs
(BI.mkCons (BI.mkPairData k a) nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose you could scan the whole list first to see if your key's already there and do the cheap cons if it isn't, but then you're paying for a possible memory saving with the CPU cost of an extra traversal.

I doubt doing two traversals for insertion is the more efficient implementation in any programming language (do people do that in any language?), and thus isn't the right thing to do in PLC either.

. toBuiltin
. PlutusTx.Prelude.map (\(k, a) -> (P.toBuiltinData k, P.toBuiltinData a))

{-# INLINEABLE uncons #-}
Copy link
Member Author

Choose a reason for hiding this comment

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

The Marlowe validator needs unsafeUncons.

This data structure is really an association list rather than a map, and should probably be named so. For an association list, it makes a lot more sense to have an uncons operation.

(k, a) = P.pairToPair hd

{-# INLINEABLE noDuplicateKeys #-}
noDuplicateKeys :: forall k a. Map k a -> Bool
Copy link
Member Author

Choose a reason for hiding this comment

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

Again, if we call this an association list, then this would be a lot more obvious.

)

{-# INLINEABLE all #-}
all :: forall k a. (P.UnsafeFromData a) => (a -> Bool) -> Map k a -> Bool
Copy link
Member Author

Choose a reason for hiding this comment

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

why not define a fold

Then fold will have to be inlined and beta-reduced, but there's no guarantee, even if it has the INLINE pragma. Recall that the INLINE pragma only makes it inline, not beta-reduce.

The code duplication shouldn't be a big deal due to CSE.

{-# INLINEABLE union #-}

-- | Combine two 'Map's.
union ::
Copy link
Contributor

@kwxm kwxm Jan 8, 2024

Choose a reason for hiding this comment

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

I'm still failing to gasp what this is doing. If I have

m1 = Map Map [(I 2,I 222),(I 1,I 111)]
m2 = Map Map [(I 2,I 999),(I 3,I 333)]

then union m1 m2 is

Map Map [(I 1,Constr 1 [I 111]),(I 2,Constr 3 [I 222,I 999]),(I 3,I 333)]

The keys that are in m1 have their values tagged with This, the keys that are in both have both values tagged with These, but the stuff that's in m2 only is untagged. Is there maybe a That missing in the code that constructs rs'?

Later: in fact, why does that even work? The signature of union says that its output has type Map k (These a b), but the map quoted above has an entry whose value isn't a These.

Ah, it's because a isn't used in the definition of Map k a.

Copy link
Contributor

@kwxm kwxm Jan 8, 2024

Choose a reason for hiding this comment

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

I was trying to think what would happen if either or both of the inputs contained duplicate keys, but my brain couldn't handle it. Also, what happens if you take a union of more than two maps? You presumably get nested Theses, which seems as if it could get out of control quickly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm also missing some context about the intended purpose of this. I see that AssocMap uses These as well, but that seems a bit weird. We have an invariant (or do we?) that maps shouldn't have repeated keys, but the use of These kind of overrides that and gives you both values if a key occurs in both inputs. You could also make union left- or right-biased, or fail if there's a key that occurs in both maps. Is the point of These to allow users to do further processing to deal with clashes themselves rather than forcing a choice on them?

Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

OK, I've finished now. I think some of it could be made more efficient, but the problem is that that would make it more complicated and less comprehensible, and hence more error-prone.

{-# INLINEABLE union #-}

-- | Combine two 'Map's.
union ::
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm also missing some context about the intended purpose of this. I see that AssocMap uses These as well, but that seems a bit weird. We have an invariant (or do we?) that maps shouldn't have repeated keys, but the use of These kind of overrides that and gives you both values if a key occurs in both inputs. You could also make union left- or right-biased, or fail if there's a key that occurs in both maps. Is the point of These to allow users to do further processing to deal with clashes themselves rather than forcing a choice on them?

tl' = go tl
in if member' k' ls
then tl'
else BI.mkCons hd tl'
Copy link
Contributor

Choose a reason for hiding this comment

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

Further to my earlier comment, think this should be something like

   else BI.mkCons (BI.mkPairData k' (P.toBuiltinData (That (P.unsafeFromBuiltinData (BI.snd hd)) :: These a b))) tl'

which is not exactly what you'd call elegant.

Map k a ->
Map k a ->
Map k a
unionWith f (toBuiltinList -> ls) (toBuiltinList -> rs) =
Copy link
Contributor

@kwxm kwxm Jan 8, 2024

Choose a reason for hiding this comment

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

Ah, this is the thing that lets users resolve clashes themselves, but it's less general than union because the values in both maps have to be of the same type. I suppose one could generalise this by making it something like

  forall k a.
  <suitable constraints> =>
  (a -> c) ->
  (b -> c) ->
  (a -> b -> c) ->
  Map k a ->
  Map k b ->
  Map k c

and then it would subsume union.

It seems unlikely that you'd really want to merge two maps with different key types though (which applies to the These-based function too).

)

res :: BI.BuiltinList (BI.BuiltinPair BuiltinData BuiltinData)
res = go rs' ls'
Copy link
Contributor

@kwxm kwxm Jan 8, 2024

Choose a reason for hiding this comment

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

Can you make this act on rs and cons the missing stuff (ie the stuff not in the overlap) directly onto ls'? I think that at the moment it's using heap memory to construct rs' as an intermediate list which is immediately traversed in order to append its contents to ls' and then discarded. If you can avoid constructing rs' then it would be more efficient in terms of both CPU and memory.

)

rs' :: BI.BuiltinList (BI.BuiltinPair BuiltinData BuiltinData)
rs' = go rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Again it might be possible to avoid constructing rs'.

@zliu41 zliu41 marked this pull request as draft February 23, 2024 02:33
@zliu41 zliu41 closed this Mar 8, 2024
@zliu41 zliu41 mentioned this pull request Apr 12, 2024
11 tasks
@ana-pantilie ana-pantilie reopened this Apr 15, 2024
@ana-pantilie
Copy link
Contributor

Closing as I am splitting this up into multiple PRs.

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.

5 participants