Explicit delegation to target class for ArrayBag

[robsyme]

Solves #5187

[netlify]

✅ Deploy Preview for nextflow-docs-staging ready!

Name	Link
🔨 Latest commit	a856025
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/66d8602e9e9dc2000877dbec
😎 Deploy Preview	https://deploy-preview-5189--nextflow-docs-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[pditommaso]

@robsyme is this PR solving the issue for you?

[robsyme]

Yup, the changes solve both the very minimal example in the new test included in this PR and also the slightly more complicated example in #5187

[robsyme]

If the point of the ArrayBag class is to provide a container for objects that order-invariant, it makes sense to ensure that hashCode and equivalence methods also reflect this property. The commented-out equals method only tests for length equivalence and a containsAll test, which would fail when testing ArrayBags with the same items with differing frequencies, e.g. [1,1,2] and [1,2,2]. I added a slighty more computationally costly equals method, but it will not fail for these cases. I also added a JoinOp unit test for this case.

[pditommaso]

This opens an interesting point: should [1,1,2] be the same of [1,2,2] ?

[pditommaso]

Also it results into an inconsistency that hashCode is honoured, but it does not equals and == operator. If you check this tests will fail

     given:
     def alpha = new ArrayBag(['abc',123,'x', 9])
     def delta = new ArrayBag([123,9,'abc','x'])

     expect:
     alpha == delta
     alpha.equals(delta)

[bentsherman]

ArrayBag itself seems over-complicated and contradictory. It's basically trying to be a List in some places and a Set in others. Instead I would argue that you should just use a List or a Set depending on what you want.

Here are all the places where ArrayBag is used:

• collect operator: produce a List if sorting is enabled, otherwise produce a Set
• groupTuple operator: same as above
• process glob input: it is an unordered collection, use a Set

The two operators should have return type Collection, so that in general order is not guaranteed in general, but can be depending on runtime conditions. Or even better, separate the sort functionality from collect and groupTuple so that they always return Set, then the sorting can be done is a separate operation.

Glob patterns in general are unordered, so it doesn't make sense to give the illusion of ordering in this case. What does files('*.fastq')[0] mean? It has no meaning

If we do all of that, we can remove ArrayBag and avoid all of this mess

[bentsherman]

Example of sorting groups explicitly after a groupTuple (source):

// ...
    | groupTuple
    | map { group, items ->
      def sorted = items
        .sort { item -> item[0].id }
        .collect { meta, csv -> csv }
      return [group, sorted]
    }

[robsyme]

Thanks for doing that analysis Ben, I agree that replacing the ArrayBag seems very sensible.

[pditommaso]

The main reason for ArrayBag is to not invalidate the cache when files are emitted in different order

https://github.com/nextflow-io/nextflow/blob/ensure-arraybag-equivalence/modules/nf-commons/src/main/nextflow/util/HashBuilder.java#L158-L159

What is tricky is that hashCode and equals is honoured when invoked by Java SDK (and therefore fixes the JoinOp), and it is not when invoked by Groovy (and pipeline code).

Given that adding hashCode and equals methods at least fixes the join implementation, I think it's fine to add them with a big notice.

I would avoid however the check for differing frequencies tho.

Do you agree?

[bentsherman]

The main reason for ArrayBag is to not invalidate the cache when files are emitted in different order

my point is that this is already supported for Set, so ArrayBag should just be replaced with Set. if the order of a collection doesn't matter for purposes of caching, it shouldn't matter for the user either.

[pditommaso]

But Set is not a List! there's a lot of stuff (including) pipeline code that rely on that

[bentsherman]

If the order doesn't matter... then they shouldn't be relying on it. Doing something like foo[0] on an ArrayBag is non-deterministic. Tbh it might explain why people have mysterious caching issues from time to time

That being said, I would be willing to defer this change to the introduction of static type checking. We'll have to clean up APIs like this anyway and then we can have a feature flag like nextflow.enable.types to make it opt-in

[pditommaso]

Sound like a reasonable plan

[bentsherman]

I forgot that Set requires uniqueness, which makes it the wrong abstraction for collect and groupTuple (should be fine for file globs though).

The Bag interface is actually the right abstraction for collecting values from a channel, as it is a generic collection, but serves as a marker for the hash builder to hash it like a set.

The problem is that ArrayBag implements List, which gives a false sense that it is ordered. If a user starts doing indexed access on an ArrayBag, they will be relying on non-deterministic behavior that could cause cryptic resume issues -- even though the ArrayBag itself is deterministic, their usage of the ArrayBag could lead to non-deterministic hashes downstream.

You mentioned frequencies as a way to test equality and that it would be expensive, but... what if we just implement Bag as a frequency map instead of an array list? Call it a HashBag. The elements already need to be hashable so it shouldn't break anything. Then the equality test is the same as for a map.

It looks like Guava even has this, it's called a Multiset:

A collection that supports order-independent equality, like Set, but may have duplicate elements. A multiset is also sometimes called a bag.

[bentsherman]

In any case, I think the migration plan is the same, since replacing ArrayBag with Multiset would drop indexing, better to change it as part of the static type API.