#2276 - evaluation of expressions on constants

[v-kabanov]

Fixed by slightly modifying a few relinq related classes, copied a couple of them from Relinq where it was not possible to override them - 1 was sealed and another had private constructor. Relinq's apache 2 license allows this as far as I understand. The issue was that NH expected Relinq to evaluate independent subtrees, but RL considers all parameters (defined in lambda expressions) as non-evaluatable and their status is pulled upwards leaving expressions on constants not evaluated. Modified visitor maintains ancestor list and checks whether parameter accepts values from evaluatable expression by analyzing its ancestors.

Fixes #2276

[v-kabanov]

Changes allowing to avoid copying and modifying the 2 classes from Relinq in the NH repo could be submitted to Relinq, but the project does not appear to be actively maintained.

[bahusoid]

could be submitted to Relinq, but the project does not appear to be actively maintained.

Maybe because it's considered perfect already ) Could you still prepare PR? At least I want to see what changes did you make to original sources.

[v-kabanov]

Could you still prepare PR?

Work in progress.

[v-kabanov]

Here are the minimal changes rolled over current develop branch: https://github.com/re-motion/Relinq/compare/develop...v-kabanov:nh-evaluate-expr-on-constants . They are the minimum (excluding unit test) required to avoid copying code into NH repository, inheriting and overriding instead, as before. NH evaluates expressions recursively and this logic is unlikely welcome in Relinq, their classes implement single pass only, based on what I've seen so far.

Some further changes are required to make relinq evaluate expressions on constants in stock version. After making the required changes I have my new test passing and 3 old tests failing (relinq tests, not NH). There are some design questions such as whether instantiation in an expression like query.Select(x => new Foo()) or query.Select(x => PropertyReturningNewInstance) should be evaluated as constant. It should probably be configurable, but for NH it is not a big issue, presumably. But even if evaluation is done in Relinq, NH will still override to implement recursion.

[v-kabanov]

pull request submitted to Relinq: https://github.com/re-motion/Relinq/pull/14

[v-kabanov]

It doesn't look like there will be any progress on the Relinq PR any time soon. There is no objections regarding the correctness of the feature implementation as such, however. So what's the plan here? The relinq library is actually not that big (at least the part used by NH), could forking it be an option considering how slowly it changes?

[bahusoid]

To me it looks like an improvement that can wait till it's accepted by Relinq considering that affected queries can be easily fixed by moving all parts that needs in memory evaluation outside of query (and it would also improve performance of such queries - as no delegate compilation would be required on each execution). Or am I wrong?

[v-kabanov]

Yes, I think you are (wrong). It could be considered an improvement if NH rejected queries with such expressions (throwing an exception). Now, as far as I know, there is no even warning in documentation saying users must not use linq expressions on in-memory collections in their queries. NH LINQ queries can easily return incorrect results. I don't think performance is a concern here; a few extra expression compilations (note, expressions/delegates are compiled anyway, just not all of them) per database roundtrip is unlikely to have a detectable performance impact.