Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle ListInitExpression in entity equality #18400

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

roji
Copy link
Member

@roji roji commented Oct 16, 2019

Closes #18379

  • We lack support for MemberMemberBinding or MemberListBinding as decided in Entity equality support for non-anonymous DTOs #16791. I think it would be good to at least handle these in top-most projection (not full support with flowing), @smitpatel let me know.
  • Theoretically there's IndexExpression which doesn't seem to be actually produced by the C# compiler. The node breaks elationalSqlTranslatingExpressionVisitor anyway, we can probably forget about it.

Haven't seen anything else but let me know if I've missed anything.

@smitpatel
Copy link
Contributor

I think it would be good to at least handle these in top-most projection (not full support with flowing),

Not a good idea. It de-stabilizes the release. 3.1 is supposed to be LTS, we should aim to make it more stable rather than adding features which contributes to bugs. We don't have support for those bindings anywhere in our code. If we "handle" them in top-level projection, we create confusion in customers when that pattern works. Yes, it could work in basic scenario cheaply but causes more chaos in all other cases. Also work-around for that is easy, by just new-ing up in query rather than relying on objects to new up. Since object is doing new allocation, it only happens when you project into specific DTO which has initialization inside class.

Theoretically there's IndexExpression which doesn't seem to be actually produced by the C# compiler.

It is produced by compiler.
a[0] is BinaryExpression with ExpressionType.ArrayIndex
a[0][0] is IndexExpression with ExpressionType.Index

That node breaks RelationalSqlTranslatingEV because relational does not support array properties at all. IndexExpression is upto you if postgre supports multidimensional array.

@roji
Copy link
Member Author

roji commented Oct 16, 2019

Not a good idea.

Ok, but doesn't that same logic apply to ListInitExpression and NewArrayExpression? Both would be supported only in the top-level projection etc.

I don't have strong feelings on this or anything, am mainly curious.

@roji
Copy link
Member Author

roji commented Oct 16, 2019

That node breaks RelationalSqlTranslatingEV because relational does not support array properties at all. IndexExpression is upto you if postgre supports multidimensional array.

PG does support multidimensional arrays, but the value of supporting this (and only in top-level projections) seems very minimal... Definitely no need to support this at this time.

@smitpatel
Copy link
Contributor

Ok, but doesn't that same logic apply to ListInitExpression and NewArrayExpression? Both would be supported only in the top-level projection etc.

You are conflating the idea of composable with top-level projection. ListInit & NewArray are not composable (except for case of constant indexer). There is no Sql translation possible and we client eval them in the top-level projection.

MemberInitExpression has MemberInfos. It is composable. That's why we have so much of code around taking care of MemberInitExpression to preserve the memberInfos and we use it to reduce into simpler expression. While memberInit is not Sql translatable, but if the reduced form is them that is something we want to support. Hence it is confusing to add support for it in top-level projection (i.e. assume they are non-composable).

In simpler words, ListInit/NewArray will never translate to server. MemberInit can be at some point so adding support in top-level projection is half baked feature with bugs.

@roji
Copy link
Member Author

roji commented Oct 17, 2019

You are conflating the idea of composable with top-level projection. ListInit & NewArray are not composable (except for case of constant indexer). There is no Sql translation possible and we client eval them in the top-level projection.

At the very least, NewArray and ListInit could be used with constant indexers (as you wrote) much like we already support composing over anonymous objects, conceptually they are very similar. Not saying there's much value there, but saying that there's no SQL translation is a bit strong.

Anyway, doesn't matter.

@roji roji force-pushed the EntityEqualityListInitExpression branch from 7e07b2b to e0a1d68 Compare October 22, 2019 13:35
@roji roji merged commit f325945 into release/3.1 Oct 22, 2019
@roji roji deleted the EntityEqualityListInitExpression branch October 22, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entity Equality: make sure all expression types are covered
2 participants