Implement & expose SkipLast & TakeLast for Enumerable & Queryable #14186

jamesqo · 2016-12-04T03:31:41Z

Fixes https://github.com/dotnet/corefx/issues/13842. Queryable implementation is courtesy of @JonHanna. The initial implementation forgoes optimizations and uses Queue as the circular buffer.

I am not quite sure what I'm doing, but I followed @JonHanna's example for #13726.

cc @stephentoub, @VSadov, @bartdesmet, @svick

JonHanna · 2016-12-04T03:37:34Z

src/System.Linq/tests/ConsistencyTests.cs

+                     nameof(Enumerable.Append),
+                     nameof(Enumerable.Prepend),
+                     "ToHashSet",
+                     "SkipLast",


Ideally (maybe we'll have to do it in phases, though I think it should be avoidable by having a P2P to queryable from system.linq.tests), this shouldn't list SkipLast and TakeLast as queryable has those.

JonHanna · 2016-12-04T03:39:47Z

src/System.Linq.Queryable/ref/System.Linq.Queryable.cs

@@ -126,6 +126,7 @@ public static partial class Queryable
        public static TSource SingleOrDefault<TSource>(this System.Linq.IQueryable<TSource> source) { throw null; }
        public static TSource SingleOrDefault<TSource>(this System.Linq.IQueryable<TSource> source, System.Linq.Expressions.Expression<System.Func<TSource, bool>> predicate) { throw null; }
        public static System.Linq.IQueryable<TSource> Skip<TSource>(this System.Linq.IQueryable<TSource> source, int count) { throw null; }
+        public static System.Linq.IQueryable<TSource> SkipLast<TSource>(this System.Linq.IQueryable<TSource> source, int count) { throw null; }


Note that I left out all versioning concerns in my commit. This will need to be #if netcoreapp11

JonHanna · 2016-12-04T03:41:02Z

src/System.Linq.Queryable/tests/System.Linq.Queryable.Tests.csproj

@@ -53,9 +53,11 @@
    <Compile Include="SequenceEqualTests.cs" />
    <Compile Include="SingleOrDefaultTests.cs" />
    <Compile Include="SingleTests.cs" />
+    <Compile Include="SkipLastTests.netcoreapp1.1.cs" />


Likewise, this is being included for all versions, but should be netcoreapp1.1 only.

JonHanna · 2016-12-04T03:42:40Z

src/System.Linq/src/System/Linq/Skip.cs

+                if (queue.Count == count)
+                {
+                    yield return item;
+                    queue.Dequeue();


Should it not yield the dequeued item rather than the current?

Yes, a quick Linqpad run shows that this is being a Skip(), not a SkipLast(). In which case there's something wrong with the tests that let this pass, too.

@JonHanna CI is failing... I mentioned in https://github.com/dotnet/corefx/issues/13842#issuecomment-264613570 that I was having trouble to get the tests to run (or even compile) locally, since all of the netcoreapp1.1 tests were being excluded. Luckily however they seem to be running in CI, and I can see the stack traces.

Note to self: e.Current should be evaluated as lazily as possible here.

JonHanna · 2016-12-04T03:53:55Z

src/System.Linq.Queryable/tests/System.Linq.Queryable.Tests.csproj

@@ -70,6 +72,11 @@
      <Name>System.Linq.Queryable</Name>
      <Private>true</Private>
    </ProjectReference>
+    <ProjectReference Include="..\..\System.Linq\src\System.Linq.csproj">


Highlighting this bit of mine, because I'm not sure I did the right thing with how this reference is in here, and if I didn't I don't want it to go unnoticed.

JonHanna · 2016-12-04T04:02:38Z

(I should probably forego commenting on optimisation for now when you're forgoing doing it for now! Deleting those comments to concentrate on correctness).

ViIvanov · 2016-12-04T17:29:31Z

src/System.Linq/src/System/Linq/Skip.cs

+
+        private static IEnumerable<TSource> SkipLastIterator<TSource>(IEnumerable<TSource> source, int count)
+        {
+            var queue = new Queue<TSource>();


What about specifying capacity for queue?

var queue = new Queue<TSource>(count);

I think if that's done, it should be clamped to a maximum. Otherwise new []{1,2}.SkipLast(10000) would create a 10000-element capacity queue to which it only adds two elements. (There are cases where we can be more confident in the count, in which case we should go ahead and use that count, though many of those would offer further optimisations akin to those currently in place for Skip() anyway).

Thanks, nice point! I agree, it can be better.

In other hand we can try to test source (for ICollection<> or IListProvider<>, as other Enumerable methods do) for count.

Also, in my practice, items.SkipLast(1) used often and we can use single TSource instead of new Queue<TSource> in this case.

@JonHanna @ViIvanov In the future I am planning to use a LargeArrayBuilder here, which since #14020 supporting limiting how many elements are allocated. Then we can call ToArray() on it and wrap a struct circular buffer around the resulting array.

@ViIvanov

Also, in my practice, items.SkipLast(1) used often and we can use single TSource instead of new Queue<TSource> in this case.

👍 Noted.

Looks like this code will not work in case count is zero.

I think the best thing when count is zero or less would be to pass to Skip() anyway; at best it passes back the source directly if the source came from Linq (this was deemed a relatively safe case for doing so in such cases, whereas with collections there could be complications due to two enumerables expected to be distinct being the safe) and otherwise would still have a lighter implementation of the same response.

@JonHanna Good idea; I've forwarded to Skip(0).

@ViIvanov Thanks for noticing that; it was a problem with TakeLast as well. Fixed for both methods.

bartdesmet · 2016-12-05T01:59:04Z

src/System.Linq.Queryable/src/System/Linq/Queryable.cs

+        public static IQueryable<TSource> SkipLast<TSource>(this IQueryable<TSource> source, int count)
+        {
+            if (source == null)
+            {


Nit: For consistency in this file, maybe get rid of { and } and the additional whitespace below?

bartdesmet · 2016-12-05T02:03:11Z

src/System.Linq/tests/ConsistencyTests.cs

-                     "Prepend",
-                     "ToHashSet"
+                     nameof(Enumerable.Append),
+                     nameof(Enumerable.Prepend),


Out of curiosity, why were Append and Prepend not added to the Queryable set of operators?

Nobody had the idea of doing so. At the time I was thinking they wouldn't be a good idea to add, but I think now I was wrong on that.

@JonHanna @bartdesmet Should I open an API proposal for that?

Not a bad idea.

bartdesmet · 2016-12-05T02:07:37Z

src/System.Linq/src/System/Linq/Skip.cs

+
+            if (count <= 0)
+            {
+                return source.Skip(0);


Is the choice of Skip(0) here arbitrary (as opposed to any other operator which would return the original sequence)? Do we refrain from returning source because we want some form of hiding of the source here?

The policy is that it's considered safe to return source if source comes from another linq operation, but not otherwise. Skip already has logic to determine that, so this can piggy-back. Skip is also naturally lighter than SkipLast, since it doesn't need a buffer queue, so this still wins in the case where it doesn't just return source.

karelz · 2017-01-04T18:27:24Z

@jamesqo just curious if it is still WIP. When do you expect it to be ready for final round of review?

ViIvanov · 2017-01-04T21:09:28Z

src/System.Linq/src/System/Linq/Skip.cs

+            using (IEnumerator<TSource> e = source.GetEnumerator())
+            {
+                while (e.MoveNext())
+                {


Why you use using and while instead of foreach here and foreach in TakeLastIterator?

@ViIvanov foreach evaluates Current right away, at the beginning of the loop. We want to evaluate it as late as possible so we don't pay an extra virtual call if iteration stops after the next yield.

Thanks, I see: this is for case when caller stops enumerating items. Nice thing!

It doesn't make much difference there, but it does mean that queue will only ever be count items in size rather than count + 1 which if count happened to be a growth threshold would be a benefit.

Having the foreach broken down allows for a further optimisation of having the loop as:

while (e.MoveNext()) { if (count == 0) { do { yield return queue.Dequeue(); queue.Enqueue(e.Current); } while (e.MoveNext()); break; } count--; queue.Enqueue(e.Current); }

Which would avoid testing the count once it had been reached.

@JonHanna You are correct, that seems like it would allow for a tighter loop (and a slightly faster comparison too, because now 0 is a comparand). I'll use your implementation in the iterator.

jamesqo · 2017-01-05T01:48:18Z

@karelz Not sure yet... I hit a roadblock during this PR because I was unable to build the netcoreapp1.1 tests locally, and I wasn't sure where to go from there. I will try in the next week to get back to this and either resolve those problems or ask for help from someone more knowledgeable about the build system.

karelz · 2017-01-05T18:06:33Z

OK, take your time, there's no rush - I just wanted to make sure we didn't drop the ball by accident here ...

jamesqo · 2017-01-08T06:09:02Z

@dotnet-bot test this please (404 on everything)

jamesqo · 2017-01-08T06:12:26Z

@karelz Alright, I finally figured out after https://github.com/dotnet/corefx/issues/14423 why I wasn't able to run the tests. Now I can actually compile and execute them on my machine without wasting CI builds (woot!), so I should be able to move forward on this when I have free time.

karelz · 2017-01-08T06:16:57Z

Great to hear. As I said, no rush, I just wanted to make sure we or the infra is not blocking you ;-)

jamesqo · 2017-01-08T21:42:06Z

@dotnet-bot skip ci please (I need to cleanup my commits)

weshaggard

Thanks for updating @jamesqo the build files look correct to me. I didn't look very closely at the actual code I will leave that for others to sign-off on.

JonHanna

The extra break in for SkipLast and the removal of "TakeLast" and "SkipLast" is from the list of excluded methods in the consistency test is the only thing in my comments I think should definitely be done. The rest are just suggestions that may not pan out.

JonHanna · 2017-01-24T11:58:06Z

src/System.Linq/src/System/Linq/Skip.cs

+                            yield return queue.Dequeue();
+                            queue.Enqueue(e.Current);
+                        }
+                        while (e.MoveNext());


Add a break; here and avoid another hit on e.MoveNext(). Aside from that being slightly wasteful, I've seen buggy enumerators that didn't behave correctly on MoveNext() following the call that returned false, so it adds a bit of defensiveness.

Nice catch. I'll modify the tests so they catch this if it's run twice.

JonHanna · 2017-01-24T12:00:18Z

src/System.Linq/tests/ConsistencyTests.cs

+                     nameof(Enumerable.Append),
+                     nameof(Enumerable.Prepend),
+                     "ToHashSet",
+                     "SkipLast",


With the recent changes, I think this should definitely work without "SkipLast" and "TakeLast" here?

JonHanna · 2017-01-24T12:12:34Z

src/System.Linq/src/System/Linq/Take.cs

+            while (queue.Count > 0)
+            {
+                yield return queue.Dequeue();
+            }


If this was foreach(TSource item in queue) yield return queue; then the lack of updating of queue on dequeuing might pay off.

Perhaps even:

private class TakeLastIterator<TSource> : IEnumerable<TSource> { private readonly IEnumerable<TSource> _source; private readonly int _count; public TakeLastIterator(IEnumerable<TSource> source, int count) { _source = source; _count = count; } public IEnumerator<TSource> GetEnumerator() { var queue = new Queue<TSource>(); int count = _count; using (IEnumerator<TSource> e = _source.GetEnumerator()) { while (e.MoveNext()) { if (count-- != 0) { queue.Enqueue(e.Current); } else { do { queue.Dequeue(); queue.Enqueue(e.Current); } while (e.MoveNext()); break; } } } return queue.GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); } }

And then a level of indirection is removed entirely, but the implementing queue is still not exposed.

(The class idea shifts the point of obtaining results from the first MoveNext() to the GetEnumerator() which differs from other methods, though perhaps acceptable since it doesn't break anything being new. Otherwise I'd still wonder about foreaching the queue rather than removing from it).

@JonHanna I am trying to forego non-trivial optimizations in this PR because it should be a no-brainer to merge. I have already written an optimized version of SkipLast with a more lightweight circular buffer: master...jamesqo:optimized.skiptake.last But I am saving that for later.

jamesqo · 2017-01-27T03:14:54Z

Alright, CI is green again. @VSadov feel free to merge

VSadov

LGTM, thanks!!

jamesqo · 2017-01-27T23:10:28Z

🎉

leandromoh · 2017-02-25T02:05:10Z

Hello!

I saw this PR was merged and I would like to know when SkipLast and TakeLast will become avaible for use ?

In the next release of the .net framework?

sorry for the ignorance.

karelz · 2017-02-25T02:26:30Z

Next release of .NET Core - the 2.0 milestone.

The big .NET Framework may eventually get it (if and when is unclear), but the issue is not marked as netfx-port-consider, so it won't be priority - it will have to wait for larger API surface comparison between .NET Core and .NET Framework.

julealgon · 2018-11-07T15:01:44Z

I just noticed these new methods (I actually thought they were our own extensions when I first found them via intellisense), but I noticed no XML comments are available to them?

Could we please ensure the new methods have proper documentation to match other LINQ methods?

karelz · 2018-11-13T00:02:08Z

@julealgon updated XML docs are a bit painpoint for us :(, esp. for new APIs - @wtgodbe @ericstj do you know what are our options?

julealgon · 2018-11-13T12:34:53Z

@julealgon updated XML docs are a bit painpoint for us :(, esp. for new APIs - @wtgodbe @ericstj do you know what are our options?

@karelz Have you considered enforcing the presence of summary tags for public-facing APIs using build-time StyleCop analyzer checks or something similar?

IMHO comments should be part of the definition of done for any contribution on standard libraries like this.

karelz · 2018-11-13T17:19:59Z

@julealgon sadly it is more involved than it seems:

We have multiple implementations of some APIs. We would have to have XML comments in ref assemblies source code (which would make them harder to read).
Many APIs do not have XML comments - we would have to baseline them.

karelz · 2018-11-13T17:20:30Z

That said, we are discussing how to make it easier in future releases and more reliable. I hope we will solve the problem by next release with new APIs (3.0).

Implement & expose SkipLast & TakeLast for Enumerable & Queryable Commit migrated from dotnet/corefx@b72def2

dnfclas added the cla-already-signed label Dec 4, 2016

JonHanna reviewed Dec 4, 2016

View reviewed changes

ViIvanov reviewed Dec 4, 2016

View reviewed changes

karelz added the area-System.Linq label Dec 4, 2016

karelz assigned JonHanna, jamesqo, VSadov and OmarTawfik Dec 4, 2016

karelz added this to the 1.2.0 milestone Dec 4, 2016

bartdesmet reviewed Dec 5, 2016

View reviewed changes

jamesqo changed the title ~~Implement & expose SkipLast & TakeLast for Enumerable & Queryable.~~ [WIP] Implement & expose SkipLast & TakeLast for Enumerable & Queryable. Dec 20, 2016

ViIvanov reviewed Jan 4, 2017

View reviewed changes

jamesqo force-pushed the st-last branch from 9f29d2d to 1457ea6 Compare January 8, 2017 21:00

jamesqo mentioned this pull request Jan 8, 2017

Fix disposal of enumerators in EnumerablePartition #14979

Merged

jamesqo force-pushed the st-last branch from 1457ea6 to 45fc0ba Compare January 8, 2017 21:41

jamesqo force-pushed the st-last branch from 45fc0ba to f89fe17 Compare January 8, 2017 21:47

jamesqo added 6 commits January 21, 2017 02:02

Expose Skip/TakeLast for Enumerable/Queryable

c5bbc12

Don't bump dir.props

38d32c2

More feedback

b93bfc6

Avoid testing the count once reached in Take

cb3654c

Fix TakeLast implementation

cb7670e

Third attempt at TakeLast

04087f7

weshaggard approved these changes Jan 23, 2017

View reviewed changes

JonHanna suggested changes Jan 24, 2017

View reviewed changes

jamesqo added 3 commits January 24, 2017 12:53

Another micro-optimization

91435b2

Respond to PR feedback

308accf

Merge branch 'master' into st-last

6fcaa47

jamesqo added 2 commits January 26, 2017 22:48

Try fix CI failures

192c290

Final CI failure?

8672da3

VSadov approved these changes Jan 27, 2017

View reviewed changes

VSadov merged commit b72def2 into dotnet:master Jan 27, 2017

jamesqo deleted the st-last branch January 27, 2017 23:10

julealgon unassigned OmarTawfik Nov 7, 2018

tomzorz mentioned this pull request Apr 8, 2019

System.Linq.Enumerable TakeLast and SkipLast don't have XML docs dotnet/dotnet-api-docs#2267

Open

jamesqo mentioned this pull request Jan 31, 2020

Proposal: Queryable.Append, Queryable.Prepend dotnet/runtime#19563

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#14186 from jamesqo/st-last

94f509c

Implement & expose SkipLast & TakeLast for Enumerable & Queryable Commit migrated from dotnet/corefx@b72def2

Implement & expose SkipLast & TakeLast for Enumerable & Queryable #14186

Implement & expose SkipLast & TakeLast for Enumerable & Queryable #14186

Conversation

jamesqo commented Dec 4, 2016 • edited Loading

JonHanna Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonHanna commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViIvanov Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelz commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonHanna Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Jan 5, 2017

karelz commented Jan 5, 2017

jamesqo commented Jan 8, 2017 • edited Loading

jamesqo commented Jan 8, 2017 • edited Loading

karelz commented Jan 8, 2017

jamesqo commented Jan 8, 2017 • edited Loading

weshaggard left a comment

Choose a reason for hiding this comment

JonHanna left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Jan 27, 2017

VSadov left a comment

Choose a reason for hiding this comment

jamesqo commented Jan 27, 2017

leandromoh commented Feb 25, 2017 • edited Loading

karelz commented Feb 25, 2017

julealgon commented Nov 7, 2018

karelz commented Nov 13, 2018

julealgon commented Nov 13, 2018

karelz commented Nov 13, 2018

karelz commented Nov 13, 2018

jamesqo commented Dec 4, 2016 •

edited

Loading

JonHanna Dec 4, 2016 •

edited

Loading

jamesqo Dec 4, 2016 •

edited

Loading

ViIvanov Dec 4, 2016 •

edited

Loading

JonHanna Jan 7, 2017 •

edited

Loading

jamesqo commented Jan 8, 2017 •

edited

Loading

jamesqo commented Jan 8, 2017 •

edited

Loading

jamesqo commented Jan 8, 2017 •

edited

Loading

JonHanna left a comment •

edited

Loading

leandromoh commented Feb 25, 2017 •

edited

Loading