Add take/takeRight/drop/dropRight to Chain

[johnynek]

I reached for drop on Chain and realized we never implemented it however, we could implement it and using internal structure it can be more efficient than using iterators/toList.

[johnynek]

this was the bug. The rest of the changes are just optimizations to the code or scalacheck improvements.

[satorg]

Thank you, these methods are really nice addition to Chain!

One thing to clarify: Chain uses Long for size. Would it make sense to make count parameter Long as well?
I realize it is unlikely that some one will ever hit Int limits in there, but size: Long implies that it is possible at least in theory.

[danicheg]

Shouldn't we eagerly exit with the newLhs if rhs.isEmpty == true?

[johnynek]

I don't think so, we have to check for empty anyway on the next call call of the loop. If we also check here we are checking twice. If we could ensure we never see Empty in the loop it would be worth it, but it would mean we would we have uglier code: we don't have a total match, we have one arm (Empty) that is unreachable. I don't think the performance win would be there and it would complicate the code.

What do you think?

[johnynek]

Okay, actually, I took this suggestion. I remembered we have NonEmpty so we can recurse on that instead, that naturally pushes the check up to the places you suggest.

I still don't think it's any faster (since the Empty was the final check and always a terminal condition), but at least it's nice to be clear in the recursion that we are only operating on NonEmptyChain.

[danicheg]

Shouldn't we eagerly exit with the newRhs if lhs.isEmpty == true?

[danicheg]

Shouldn't we eagerly exit with the Empty if rhs.isEmpty == true?

[satorg]

I guess dropped can have size == 1 here, but according to the Wrap's invariant:

cats/core/src/main/scala/cats/data/Chain.scala

Lines 937 to 945 in 765f781

	/*
	* Invariant: (seq.length >= 2)
	* if the length is zero, fromSeq returns Empty
	* if the length is one, fromSeq returns Singleton
	*
	* The only places we create Wrap is in fromSeq and in methods that preserve
	* length: zipWithIndex, map, sort
	*/
	final private[data] case class Wrap[A](seq: immutable.Seq[A]) extends NonEmpty[A]

So it looks like in order to meet the invariant criteria it should be something like

val wrapped = if (dropped.size > 1) Wrap(dropped) else Singleton(dropped.head)

The same concern is for dropRight and maybe take/takeRight methods.

However, perhaps it would be even better to add a general-purpose method to object Chain that could guarantee the invariant, e.g.

private def wrapOrSingleton[A](seq: Seq[A): NonEmpty[A] =
  if (seq.size > 1) Wrap(seq) else Singleton(seq.head) // assume seq is never empty here

Also, it looks like this note in the docs to Wrap:

The only places we create Wrap is in fromSeq and in methods that preserve length: zipWithIndex, map, sort

is not applicable anymore.

[satorg]

UPD.: actually, take/takeRight don't seem to be affected. But drop/dropRight do. For example:

Chain.fromSeq(Seq(1, 2)).drop(1)

results to Chain.Wrap(2)

[johnynek]

good catch. I fixed the comment and added the branch. I did use lengthCompare(1) rather than branching on .size since the former is more efficient for List (or other Seqs that don't store the length).

[johnynek]

I think the catsNative has been giving spurious failures, so I think this is ready now?

[satorg]

Thank you!

[satorg]

Hi @johnynek , @danicheg, sorry for the ping, just to clarify with you guys – where do we stand on this?

The reason I'm asking – I'd like to kick off v2.13.0, but I really like this feature and think it would be nice to have it included. However, if there are some concerns or work to-do for this PR, then perhaps it makes sense to release without this one and postpone it for a follow-up release. So WDYT?

[johnynek]

I think it's ready. I just didn't want to merge my own PR without the second +1.

[danicheg]

It's definitely good to go 👍🏼

[johnynek]

It might be nice to have splitAt also. You can implement it with drop and take but I think you can do a single pass with a better implementation.