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

Query: Allow EF.Default for value types #28973

Closed
wants to merge 1 commit into from
Closed

Conversation

smitpatel
Copy link
Contributor

Resolves #28921

@AndriySvyryd
Copy link
Member

How confident are you in this?
Should we revert #28970 and merge this to 7.0?

@smitpatel
Copy link
Contributor Author

The SqlFragmentExpression was supposed to be putting arbitrary token in SQL tree. We only used it for Count(*). For other usages we generally decided to go for more structured custom expression like AtTimeZoneExpression. When we started using for DEFAULT it enforced the actual type on the token has meaning so typeof(string) is wrong. This change still doesn't make SqlFragment an arbitrary token because it doesn't take type mapping and it cannot be put in projection directly. We should consider changing that in future perhaps to assign actual type/typemapping to it. (Default node can take correct type mapping then).
Because it has typeof(object) in EqualsTranslator we need to account for it. We pass Constant/Parameter because they can have object type due to the method signature. SqlFragment just became another one. Since we don't want to parse the string inside it, change kinda makes sense that we would just generate equality in server. The returning false in this translator is kind of an optimization and also way to avoid issues around comparing mismatching type and type inference. There wouldn't be much harm to sending it to server and let server evaluate to false as long as our inference doesn't crash in between.
If above changes feels risky then we can go with custom DefaultExpression which can take type from EF.Default<T> so everything else would work just like any other equality.

In random chance event, the SqlFragment had string type and the test we added for default had string type too so it just worked. This set of change make it work for all types now. Do you have any particular scenarios in mind or possibility of variation than what we have now which may fail? Possibly there are cases where things would fail randomly if user decide to use this API in wrong place. Not sure if DEFAULT can appear anywhere other than Update command (cc: @roji). If we want to guard on that then we can also add SetPropertyToDefault and remove EF.Default so people cannot use in wrong context.

@ajcvickers
Copy link
Member

Note from triage: wait for work to start on main before merging.

@smitpatel smitpatel closed this Sep 8, 2022
@smitpatel smitpatel deleted the smit/defaulting branch October 13, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants