ravendb / ravendb.contrib

External Contributions to RavenDB
http://ravendb.net
MIT License
30 stars 14 forks source link

PagedList Implementation for IDocumentQuery #19

Closed khalidabuhakmeh closed 11 years ago

khalidabuhakmeh commented 11 years ago

PagedList is an awesome NuGet package and I've been using it with a lot of success in my RavenDB projects. The only issue is when you are using a LuceneQuery it stops to work. So I implemented a DocumentQueryPagedList that properly pages off of IDocumentQuery objects.

Example:

var result = session
    .Advanced
    .LuceneQuery<Test>()
    .OrderBy(x => x.Id)
    .ToPagedList(1, 10);

Paged List out of the Box (works with no needed modifications):

var results = session
    .Query<Test>()
    .OrderBy(x => x.Id)
    .ToPagedList(1, 10);

Hope you find this helpful, I know I did.

Khalid

mattjohnsonpint commented 11 years ago

Hmmm.. I am slightly confused. PagedList library seems to be about splitting up something that is a big list of items into separate pages. But this is already functionality provided by .Skip() and .Take(). Also, Raven already does paging of results, so using something like PagedList is going to give you back a "sub-page" within the page you got.

I took a quick look at the implementation of PagedList and I'm not too thrilled about it. He starts off by taking a .Count() over the set - which will force an enumeration right there. Then he does a lot of math to figure out what to pass into Skip() and Take() and ultimately calls ToList() on it, causing a second enumeration. Multiple enumerations of an IEnumerable or IQueryable is a bad thing.

Also, in Raven - doing a .Count() is not going to work well. You need to use .Statistics() and look at .TotalResults and .SkippedResults to do anything useful. See the docs on paging.

Lastly - Raven.Client.Contrib should stay free of third-party dependencies other than Raven.Client. I just saw that Miniprofiler was still in there and shouldn't have been - that code was moved into it's own project. I removed that reference.

If you want to try implementing a .ToPagedList() method of your own, without the dependency and taking raven's paging, statistics, skipped results, etc. into account - that would be welcome. Thanks.

khalidabuhakmeh commented 11 years ago

Could you update the ReadMe to state that no external dependencies should be taken other than Raven so it is clearer to other contributors. Thanks.

khalidabuhakmeh commented 11 years ago

He starts off by taking a .Count() over the set - which will force an enumeration right there.

That is not true. If you have a properly implemented IQueryable interface (like RavenDB has) it should issue a specialized count query, which is not an enumeration. It is a low cost operation, although not as low cost as using the statistics object in Raven.

I've confirmed Count() works fine with this code:

var count = session.Query<Test>().Count();
Assert.True(count == 1000);

The reason I am using the PagedList library from NuGet is there are a lot of other libraries building off of it, http://nuget.org/packages?q=PagedList

It just makes sense to build off of an already popular library.

mattjohnsonpint commented 11 years ago

Ah - I didn't realize until now that raven translates .Count() to use the query statistics. Good call. I thought it might be just pulling the count from the one page.

I am seeing some strange results though with the stats being wrong in general with large numbers of documents, but I think this is unrelated. I will get those to the raven issue tracker separately.

Regarding dependencies - it's only Raven.Client.Contrib that I don't want to force the dependencies on. It is just there to extend the Raven.Client project. It will ultimately become a nuget, and I don't want everyone that takes it to be force-fed a dependency requirement for some part of the library they didn't care about.

There are other projects in the solution that do have dependencies. If you wanted to create a Raven.Client.Contrib.PagedList project - you could do that. It seems a bit odd for something so small though - but I would accept it there. Thanks.

CMircea commented 11 years ago

Why do you even need a Raven PagedList implementation? Just take PagedList and use it directly; if it doesn't accept IQueryable, change the constructor to take IQueryable and make an IEnumerable one that goes via .AsQueryable().

I used PagedList just fine.

On Wed, Jan 9, 2013 at 11:13 PM, Matt Johnson notifications@github.comwrote:

Ah - I didn't realize until now that raven translates .Count() to use the query statistics. Good call. I thought it might be just pulling the count from the one page.

I am seeing some strange results though with the stats being wrong in general with large numbers of documents, but I think this is unrelated. I will get those to the raven issue tracker separately.

Regarding dependencies - it's only Raven.Client.Contrib that I don't want to force the dependencies on. It is just there to extend the Raven.Client project. It will ultimately become a nuget, and I don't want everyone that takes it to be force-fed a dependency requirement for some part of the library they didn't care about.

There are other projects in the solution that do have dependencies. If you wanted to create a Raven.Client.Contrib.PagedList project - you could do that. It seems a bit odd for something so small though - but I would accept it there. Thanks.

— Reply to this email directly or view it on GitHubhttps://github.com/ravendb/ravendb.contrib/pull/19#issuecomment-12066913.

Sincerely, Mircea Chirea

khalidabuhakmeh commented 11 years ago

You are right about PagedList working fine for IQueryable (although it could be more efficient like matt points out), but the issue arises when you want to use PagedList with a Lucene Query. It just doesn't work unless you page before hand and pass it into the constructor which defeats the purpose.

Matt is also right about it being small, so It is probably not worth adding in if people aren't screaming for it. I have the code on my branch if anybody wants it.

mattjohnsonpint commented 11 years ago

Nevermind what I said about getting strange results. I hadn't updated my test project to 2230 yet. I was hitting on an old bug. Counting works fine. Again - thanks for that tidbit of knowledge.

mattjohnsonpint commented 11 years ago

@khalidabuhakmeh I took another pass through your code. Now that I understand better, I actually like what I see quite a bit. I just don't want to introduce the dependency.

Actually, I think the dependency should be inverted. To me - this seems like an add-on for PagedList to get it to support RavenDB lucene queries. Perhaps it would do well as PagedList.RavenDB in your own project/nuget rather than as an extension to ravendb?

mattjohnsonpint commented 11 years ago

To clarify - if you want it in Raven.Contrib, then make a separate project and I will accept it. I just think it could go either direction of the dependency.

troygoode commented 11 years ago

(disclaimer: I'm the creator of the PagedList library)

@khalidabuhakmeh: For something like a Lucene Query I would recommend using the StaticPagedList class that ships with the library. You do lose some of the niceties of having your paging math calculated for you, but what you gain is a uniform IPagedList interface, and HtmlHelpers that are able to render markup based upon that interface.