Feature/stateless filtering and transforming

[JakenVeina]

Implemented one of the operators described in #758 as well as an equivalent transform operator.

Also made a few housekeeping changes, but I've split those out into separate commits, so they can more-easily be reviewed and/or excluded if others don't agree.

Benchmark results, highlighting the performance of the new operators, versus existing ones:

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
Filter	293.3 μs	5.82 μs	7.78 μs	1.00	0.00	76.1719	311.27 KB	1.00
FilterByValues	229.8 μs	3.16 μs	2.80 μs	0.78	0.02	57.3730	235.05 KB	0.76

Method	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen0	Gen1	Allocated	Alloc Ratio
Transform	457.1 μs	8.79 μs	18.15 μs	448.8 μs	1.00	0.00	153.3203	25.3906	627.89 KB	1.00
TransformByValues	261.1 μs	5.13 μs	4.28 μs	260.8 μs	0.55	0.03	57.1289	-	235.04 KB	0.37

[dwcullop]

I'm in favor of these additions, but I do have a small issue with the names. FilterByValues is confusing. Values makes it seem like each one is going to be filtered by multiple values. At the very least, it should be singular FilterByValue but I think that ValueFilter might be an even better choice.

These are going to require some documentation to ensure that the provided filter/transform functions are completely deterministic and very efficient because they'll be invoked more times than one might expect.

[JakenVeina]

I'm not sold on the names, either. Other options include FilterDeterministic, FilterSlim, FilterValues. I'm gonna say ValueFilter is a no-go, for me, because there's a lot of discoverability value in having it start with Filter, so it shows up right alongside the other Filter operators.

Regarding documentation, is the documentation I already added insufficient for you?

[dwcullop]

I'm not sold on the names, either. Other options include FilterDeterministic, FilterSlim, FilterValues. I'm gonna say ValueFilter is a no-go, for me, because there's a lot of discoverability value in having it start with Filter, so it shows up right alongside the other Filter operators.

Agreed... Although Intellisense finds anything with "Filter" in the name, the things starting with "Filter" are shown first. So... No ValueFilter and probably not anything with "Value" because that word has special meaning in DotNet and, to some, it may imply it somehow uses a stack-based value type (ala ValueTask), which is not necessarily the case at all.

Of the remaining choices, I like FilterSlim. FilterDeterministic is okay, I guess, but it's rather long and it isn't immediately clear to what the "Deterministic" refers. Other ideas:

• FilterInline
• FilterChangeSet (because it just filters the changeset without the intermediate cache)
• FilterFilterFilter (okay, I guess I'm out of ideas for now)

I feel like it might be better to get the opinions of someone who isn't familiar with DD to find a name that people can know what it does intuitively. My intuition is ruined by familiarity. Maybe we could take a survey in the Rx slack?

Regarding documentation, is the documentslation I already added insufficient for you?

Yes, your documentslation is fine... Great, actually, but the not everyone will see the XML comments, so a Wiki article might also be helpful.

[JakenVeina]

In a perfect world, I'd say it should be Filter() with all the OTHER overloads being FilterWithCaching(), so maybe FilterWithoutCaching() as another option?

a Wiki article might also be helpful.

I can get behind that.

[dwcullop]

In a perfect world, I'd say it should be Filter() with all the OTHER overloads being FilterWithCaching(), so maybe FilterWithoutCaching() as another option?

Of the names we've examined so far, my favorite is FilterSlim. It isn't long too long and it's clear it does something less than the other Filter.

[dwcullop]

Maybe your PR title has the right of it... FilterStateless? I dunno.

[RolandPheasant]

I've been sat on this for a while so I could think long and hard about it and I think using optimiser options (see my comments against the code) could be a good solution.

The only draw back would be yet more optional overloads, unless of course we introduce an overload which accepts the transform / filter functions + a record containing all the additional options. This would be similar to my suggestion on #819 and also similar to the new binding options.

[RolandPheasant]

Is this the same a Convert?

See https://github.com/reactivemarbles/DynamicData/blob/main/src/DynamicData/Cache/ObservableCacheEx.cs#L1014.

Maybe that could be resurrected?

[JakenVeina]

Uhhh, yep, that looks pretty much the same, save for a few technical optimizations.

I think it's worth considering that .Convert() has a bit of a discoverability problem, then. So, we could consider options for addressing that:

1. Add TransformByValues() (or whatever name we agree on) anyway, and have it and .Convert() both call the same implementation behind the scenes.
2. Option 1 but also mark .Convert() as Obsolete, to be removed some day.
3. Just try and address it with some new Wiki content, as Darrin already suggested. In which case, I would still swap .Convert() over to the implementation I wrote here.

[RolandPheasant]

I wonder whether we can achieve this overload using an optimizer. For example Sort accepts SortOptimisations which amongst other options allows the consumer to specify SortOptimisations.ComparesImmutableValuesOnly.

If something similar was used for Transform, or for Filter we could use different code paths in the background and eliminate the need to new named overloads.

[JakenVeina]

I could probably get behind that, in theory, but part of the problem is that this optimization is ONLY applicable for certain scenarios of filtering and transforming. Like, any of the overloads that take IObservable<> of something to control filtering flat-out cannot use this optimization.

So, as an example, if we were to define FilterOptimizations.DontCacheItems, we'd have to do one of a few things:

1. Only define one overload to take a FilterOptimizations paramter, the one where the filter predicate is just Func<T, bool>. With this, if we ever wanted to add more optimizations for other overloads, we'd have to write SEPARATE enums for those overloads.
2. Throw an exception if you try and specify FilterOptimizations.DontCacheItems with an overload that supports dynamic filtering, E.G. IObservable<Func<T, bool>>.
3. Just silently have FilterOptimizations.DontCacheItems do nothing for overloads that support dynamic filtering, leading users to think they're getting a performance boost when they're not.

On a separate note (and this comes with a grain of salt, because I have not done a DEEP dive into it, just seen it on the surface) my current impression of how sorting works in DynamicData is that I don't really like it. I don't like how much seemingly-unnecessary stuff there is inside ISortedChangeSet<>, and SortOptimizations is part of that. I have found that the "extra" stuff in ISortedChangeSet<> makes it impossible to implement some of the most common querying scenarios supported by LINQ. All that being said, I'm initially very wary about the idea of adding FilterOptimizations in the same manner as SortOptimizations.

[RolandPheasant]

@JakenVeina regarding naming just go with your gut - dealers choice

One more suggestion however is maybe FilterImmutable / TransformImmutable after all there is a GroupByImmutable which although has a slightly different behaviour it may provide consistency.

Let me know when this is ready and let's merge.

[JakenVeina]

If .GroupByImmutable() already exists, then that sounds like a winner.

And my gut says .TransformImmutable() fits better than .Convert() for the same reasons, so I think I'l go with the "alias for now, deprecate for later" approach for that one.

[dwcullop]

I can deal with the Immutable names. TransformImmutable / FilterImmutable, etc.

[JakenVeina]

@RolandPheasant I've updated namings, as discussed, and re-wrote the documentation, after some feedback from @Insire.

Since the older .Convert() operator is already marked as obsolete, I left it as-is, to be removed eventually.

We should be good for a merge.

[Insire]

I'm quite happy with the new xml comments, which was a major point of discussion i had with @JakenVeina

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.