Shuffle-based drop duplicates produces incorrect result with ``shuffle="p2p"`` about dask HOT 10 OPEN

cc @phofl

from dask.

Related to dask-contrib/dask-expr#526

from dask.

I am fine with switching to keep=None, I am also fine with warning if the user gives anything other than the default for keep and we would default to p2p shuffle, but I am -1 on choosing task based shuffling without the user specifically requesting it in these scenarios. Users should actively request tasks

from dask.

but I am -1 on choosing task based shuffling without the user specifically requesting it in these scenarios. Users should actively request tasks

Yeah, I suppose can understand that. I do think it is best to use the "stable" shuffle algorithm unless the user explicitly asks for "tasks". In that case, my only suggestion would be to warn the user that the results are likely to be "wrong" (as you already mentioned).

from dask.

@hendrikmakait with similar indexing foo as for arrays we should be able to make P2P stable, shouldn't we?

from dask.

Sure, guaranteed ordering is definitely possible. I'm not sure if indexing foo is sufficient or if we also need to make changes to guarantee ordering within shards but we can look into this.

from dask.

or if we also need to make changes to guarantee ordering within shards but we can look into this.

It seems like the groupby-based sharing should should preserve ordering on its own, no?

If there is motivation to avoid any modifications to the general "p2p" logic, what about the naive approach of (optionally) adding a "__source_partition" column before the shuffle? If the shards themselves are ordered, then a uint16 column is probably all you need to sort the output partitions in most cases.

from dask.

It seems like the groupby-based sharing should should preserve ordering on its own, no?

How do you mean?

from dask.

How do you mean?

Sorry, I'm probably "thinking out loud" a bit too much. I was mis-remembering that the initial "sharding" in shuffle_transfer/split_by_worker was groupby based.

from dask.

For the drop_duplicates I believe there is a simple way to fix this, see #10722 (I may need to be a little more careful about when that sort is appropriate but I think it's not harmful)

I opened dask/distributed#8421 to discuss the possibility to make P2P stable but I don't consider this high prio right now

from dask.