MudDataGrid: Make AggregateDefinition work with nullable values and cover with tests

[ScarletKuro]

Description

This PR allows to have nullable types in AggregateDefinition. Original LINQ methods are able to work with int?, long?, decimal? etc when calculating Min, Max, Average, Sum. Previous implementation would throw exception.
For this I changed ChangeExpressionReturnType<T, decimal> to ChangeExpressionReturnType<T, decimal?>.
I'm also kinda curious why ChangeExpressionReturnType<T, object> was used, is there any reason behind it?

I redid the cache. It's calculating the hash of the expression and stores it basically in a dictionary.
Previous one had a drawback, if you change the expression, but do not change the AggregateType, then it would evaluate the previous expression for this type, now you can reuse the same instance of AggregateDefinition without a problem.

I know I had a really brief talk with adameste where I proposed the idea of expression cache class, and he had valid points against it. But I still decided to give it a try, because: we can make it not centralized, about the performance (slow read) - AggregateDefinition is not a hot spot class, as well we can drop ConcurrentDictionary and use something like DictionarySlim that has a single lookup for GetOrAdd and performs better for value type keys. I didn't go for it because it's much more code and if only one class is going to use it then it's an overkill.
Ofc this is all negotiable whatever we need this or not.

I made many unit tests to make sure everything works correctly, they should be readable and pretty straightforward. All existing unit tests that covered this before are also working.

I made a dedicated folder "DataGrid" in test project, since the DataGrid test file is already big, and since the component is really huge and important I would encourage in future to split it and test it class by class instead.

How Has This Been Tested?

New unit tests + existing unit tests

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[ScarletKuro]

@tjscience @henon

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.29 🎉

Comparison is base (2fcee97) 91.17% compared to head (daf22b5) 91.46%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #6515      +/-   ##
==========================================
+ Coverage   91.17%   91.46%   +0.29%     
==========================================
  Files         392      393       +1     
  Lines       14826    14874      +48     
==========================================
+ Hits        13517    13605      +88     
+ Misses       1309     1269      -40     

Impacted Files	Coverage Δ
...dBlazor/Components/DataGrid/AggregateDefinition.cs	100.00% <100.00%> (+60.60%)	⬆️
...udBlazor/Utilities/Expressions/ExpressionHasher.cs	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tjscience]

@tjscience @henon

I will take a look. Thanks, @ScarletKuro !

[tjscience]

@ScarletKuro, the ChangeExpressionReturnType is used to safely change the return type of an expression. It is just a helper so that you do not have to have that code repeated for each aggregate type expression.

[ScarletKuro]

the ChangeExpressionReturnType is used to safely change the return type of an expression.

This I understand. I was more curious why for Min and Max expressions was used ChangeExpressionReturnType<T, object>() instead of ChangeExpressionReturnType<T, decimal>() why object over decimal when the rest used decimal? Is there a reason behind it? I'm asking because I changed it to decimal and was wondering if it breaks particular any case, but I personally haven't found any regression / issue with that and test didn't find any too,

[tjscience]

the ChangeExpressionReturnType is used to safely change the return type of an expression.

This I understand. I was more curious why for Min and Max expressions was used ChangeExpressionReturnType<T, object>() instead of ChangeExpressionReturnType<T, decimal>() why object over decimal when the rest used decimal? Is there a reason behind it? I'm asking because I changed it to decimal and was wondering if it breaks particular any case, but I personally haven't found any regression / issue with that and test didn't find any too,

Hmm, I can't remember now, but it might have been because the MinMax aggregate is shared between min and max.

[ScarletKuro]

I updated the HashCode, now it's more clean. I also added tests to HasVisitor, it should cover all cases, the coverage increased too.
About the performance: sadly, calculating the hash of expreission adds overhead of 500ns when GetValue is called and type is Min, Max, Sum, Average.

I guess it really depends on use case. I'm not sure how AggregateDefinition is used in real world.
Old cache can cache only one expression per type, this one(new) can have multiple.
If you use same instance of AggregateDefinition and feed it with various expressions and types, then this new approach will significantly win because it can cache them all while old one has to compile the expression again which is 27x times slower.
But if you use it as one time thing, and the data never mutates then the old approach is better,

The last word is yours.

[tjscience]

I updated the HashCode, now it's more clean. I also added tests to HasVisitor, it should cover all cases, the coverage increased too. About the performance: sadly, calculating the hash of expreission adds overhead of 500ns when GetValue is called and type is Min, Max, Sum, Average.

I guess it really depends on use case. I'm not sure how AggregateDefinition is used in real world. Old cache can cache only one expression per type, this one(new) can have multiple. If you use same instance of AggregateDefinition and feed it with various expressions and types, then this new approach will significantly win because it can cache them all while old one has to compile the expression again which is 27x times slower. But if you use it as one time thing, and the data never mutates then the old approach is better,

The last word is yours.

I don't think 500ns per call is anything to worry about here. The only time it would even possibly be an issue is when there are way too many groups expanded. Looks like the benefits outweigh the negligible perf hit.