Limit with DataList::filterByCallback is inconsistent.

[silverstripe]

Limit with DataList::filterByCallback is inconsistent.

[dhensby]

That will respect the limit better, but it would not be performant as it's still fetching (and presumably instantiating) all records from the database.

Yes, I don't think the solution should be to just discard the limit off the original query... Whilst this shouldn't instantiate all the objects, it could load thousands, hundreds of thousands, etc of DB rows into memory only to return 10.

It may be better to go through the results in chunks of 2 x limit until the limit is hit. What happens when we start introducing offset logic? For that to work properly, we always need to start from the beginning and not rely on any DB level offsets.

Ultimately, the filterByCallback method is always going to be crude and, as datasets get large, be completely unreliable as a method for filtering large sets. I think we have two choices, one of which is just to leave it as it is and have it be a known limitation of the feature, or we improve it but provide lots of guidance/warnings about how it really shouldn't be used for any large sized datasets.

[kinglozzer]

I think we have two choices, one of which is just to leave it as it is and have it be a known limitation of the feature

This gets my vote. I think time on this would be better spent elsewhere given that filterByCallback() is a bit of an anti-pattern

[christopherdarling]

I'm happy enough with it being left as is also. I wonder if it's just worth documenting somewhere these limitations/gotchas and to consider the order of the use of this due to potential performance impact

[GuySartorelli]

Documenting this as a known limitation sounds like a good idea.

[GuySartorelli]

I was thinking of resolving this by removing the limit from a clone of the DataList prior to running the callback filter loop, and then breaking the loop once we have the limit number of items. That will respect the limit better, but it would not be performant as it's still fetching (and presumably instantiating) all records from the database.

I haven't had time to really dive into this so I'm raising it as an issue for now.

[GuySartorelli]

Moved to docs repo since the concensus is to update the docs.