Improve TruncateTime canonical function performance #467
Conversation
| { | ||
| if (isDateTimeOffset) | ||
| { | ||
| throw new NotSupportedException(Strings.SqlGen_CanonicalFunctionNotSupportedPriorSql10(e.Function.Name)); |
There was a problem hiding this comment.
This looks like a break.
There was a problem hiding this comment.
For PreKatamai, the sql produced is CONVERT(datetimeoffset, CONVERT(VARCHAR(255), expression, 102) + ' 00:00:00 ' + Right(convert(varchar(255), @arg, 121), 6), 102), which is invalid for PreKatamai because the datetimeoffset data type didn't exist in Sql Server < 2008.
Because of this I don't believe that this is actually a break.
There was a problem hiding this comment.
I agree this should be fine. It is also possible that this just caused an error on the server, but it is also possible that it is blocked by something upstream in the query pipeline.
divega
changed the title
|
I believe the original CodePlex PR was submitted by @brandondahler, in case he is interested in following up. |
|
Thanks for the tag, answered inline. |
|
@divega This look okay to you? |
There was a problem hiding this comment.
@ajcvickers I think we could take it as it is, but I spent a few minutes and came back with some minor suggestions.
| isDateTimeOffset = true; | ||
| } | ||
| else | ||
| if (!isDateTimeOffset && typeKind != PrimitiveTypeKind.DateTime) | ||
| { | ||
| Debug.Assert(true, "Unexpected type to TruncateTime" + typeKind.ToString()); |
There was a problem hiding this comment.
Nit: move the condition inside the Debug.Assert call. Before this was free because it was the else part of an if we already had, but that is removed, no reason to evaluate the condition in a release build.
There was a problem hiding this comment.
Funnily enough, Debug.Assert(true, ...) is also a GNDN. Will be properly fixed as well.
| { | ||
| result.Append("+ ' 00:00:00 ' + Right(convert(varchar(255), "); | ||
| if (!isDateTimeOffset) |
There was a problem hiding this comment.
Prefer code clarity: I don't think the few lines that are common are worth the entanglement of the two translations.
| @@ -514,7 +514,7 @@ public void CanonicalFunction_truncatetime_translated_properly_to_sql_function() | |||
| using (var context = GetArubaCeContext()) | |||
| { | |||
| var query = context.CreateQuery<DateTime>(@"SELECT VALUE Edm.TruncateTime(A.c5_datetime) FROM ArubaCeContext.AllTypes AS A"); | |||
| Assert.Contains("CONVERT", query.ToTraceString().ToUpperInvariant()); | |||
| Assert.Contains("DATEADD(D, DATEDIFF(D,", query.ToTraceString().ToUpperInvariant()); | |||
There was a problem hiding this comment.
We only have canonical function tests for SQL CE? 🤦♂️ Glad we have coverage for this in https://github.com/aspnet/EntityFramework6/blob/master/test/EntityFramework/FunctionalTests/ProductivityApi/DbFunctionScenarios.cs.
|
Changes made in https://github.com/brandondahler/EntityFramework6/tree/truncate_time, unable to actually update this pull request because it migrated from @maumar from CodePlex. Let me know if you'd like me to open a new PR or if one of the members of the repository will take care of it. Was able to find a good place to put a net-new test for the updated code. |
|
I think a new PR would be good. That way the commits will be correctly assigned to you. |
|
Superseded by #948. |
Ported from CodePlex