Ensure that groupKey objects are treated as their groupTarget for the purposes of joins and combines

[robsyme]

When using operators like join or combine, if one side of the join uses a plain value (e.g., a string) and the other side uses a GroupKey (wrapping the same string), the join may fail to match them—even though they represent the same logical key.

This is because, although GroupKey implements equals and hashCode to delegate to its target, there are subtle issues in Groovy/Java equality and hash semantics, especially when comparing a GroupKey to a plain value from the other side. This can lead to unpredictable or inconsistent join results, as described in #4104.

v = 'BATCH1'
gk = groupKey(v, 3)
println v == gk // false
println gk == v // true

This asymmetry can cause joins to miss matches, depending on which side emits the GroupKey. The ordering of items on left or right can depend on the order of items in channels, leading to difficult-to-debug timing issues. This issue affects the join and combine operators which will do these sorts of equality checks.

Both JoinOp and CombineOp use the makeKey utility (which creates a KeyPair) to extract keys from tuples for matching. By ensuring that all keys are "unwrapped" from GroupKey to their target value, we guarantee that:
A key 'BATCH1' and a key groupKey('BATCH1') are treated as the same for the purposes of joining or combining.
This removes the asymmetry and inconsistency in key matching, regardless of which side emits a GroupKey or a plain value.

[netlify]

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	145e24d
🔍 Latest deploy log	https://app.netlify.com/projects/nextflow-docs-staging/deploys/6846d745be715c000993862c

[pditommaso]

This sounds very reasonable, it may be worth adding a unit test

[robsyme]

Agreed. Will fix that today.

[pditommaso]

Not strictly required, right?

[robsyme]

If the [entry] is added directly to the result.keys List, it doesn't pass through the safeStr method, and thus would miss the new instanceOf GroupKey check.

If we wanted to be strict about it, we could make the keys List private to make sure addKey is the only way to add a KeyPair, but I'm not sure it's worth the extra complexity. KeyPair is not used broadly in the codebase.

[robsyme]

Looking at this a bit more closely this morning, I see that this change does "unwrap" the groupKey if the channel passes through the join or combine.

The options are:

1. Current state (before this PR): Don't unwrap automatically anywhere - the user is responsible for calling getGroupTarget(). Inside of join and combine, groupKey("foo") == "foo" but "foo" != groupKey("foo"), leading to timing-dependent bugs if joining/combining one wrapped channel with an unwrapped channel (see example in Channel emits differently depending on order of dependent task completion #4104).

• wrapped.join(unwrapped) → empty if the unwrapped value arrives first
• wrapped.join(unwrapped) → wrapped values if the wrapped value arrives first.
• groupTuple(wrapped) → wrapped

2. We remove the groupKey("foo") == "foo" equality. This would give us more consistent behaviour, but I'm not sure if we depend on this equality elsewhere in the codebase. A lot of users use dump and view to inspect the values, and because the wrappend and unwrapped values toString() exatly the same, users may be confused why their joins are not working.

• wrapped.join(unwrapped) → empty
• groupTuple(wrapped) → wrapped

3. This PR: We solve the commutitivity problem by always returning unwrapping in combine and join. The groupKey lifetime extends until it hits join or combine.

• wrapped.join(unwrapped) → unwrapped
• groupTuple(wrapped) → wrapped

4. We unwrap on groupTuple, effectively "consuming" the groupKey when used as a group.

• wrapped.join(unwrapped) → empty if the unwrapped value arrives first
• wrapped.join(unwrapped) → wrapped values if the wrapped value arrives first.
• groupTuple(wrapped) → unwrapped

5. We do both options 3 and 4. The groupKey lifetime extends until it hits join, combine, or groupTuple

• wrapped.join(unwrapped) → unwrapped
• groupTuple(wrapped) → unwrapped

I think option 3 (the PR) is the cleanest without introducing major breaking changes. The only "breaking" change this introduces is if a workflow is relying on a groupKey being passed through a join/combine with the matching unwrapped value. Such a workflow is already in a fragile timing-dependent state anyway, so I think it's ok draw attention to this by introducing this "breaking" change.

[pditommaso]

I need AI to summarise this 😆

[pditommaso]

In my view this could be managed, unwrapping temporary the key for the requirement comparison/indexing operation, but continuing to emit as wrapped GroupKey. This essential because this was designed to traverse a chain of operators

[robsyme]

If so, we may have to consider the pathological case where two groupKeys are joined, but the groupKey contains different size values 😬

[pditommaso]

How that would happen in practice?

[bentsherman]

Now I hope you see why I would rather deal with this in operators v2

[robsyme]

How that would happen in practice?

A fair concern. This is very unlikely, and I can't imagine anybody doing it intentionally. Maybe we can just issue a warning in this very rare circumstance.

Now I hope you see why I would rather deal with this in operators v2

Yes, Ben :D I would still like to find a neat solution for existing users. If you work on v2, I'll give some thought to an interim solution here. Nextflow is a path to reproducibility, so it would be great to help users avoid timing-dependent workflow bugs that break that reproducibility guarantee.

[robsyme]

Ok, I've fixed it to ensure that the groupKey is preserved if combined with the target. I'll accept that preserving the key comes at the cost of some additional complexity 😬