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.

Sign up for GitHub

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

Jump to bottom

Improve TruncateTime canonical function performance #467

Closed
wants to merge 1 commit into from
Closed

Improve TruncateTime canonical function performance #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 40 additions & 26 deletions src/EntityFramework.SqlServer/SqlGen/SqlFunctionCallHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,49 +1285,63 @@ private static void AppendConvertToVarchar(SqlGenerator sqlgen, SqlBuilder resul

// <summary>
// TruncateTime(DateTime X)
// PreKatmai: TRUNCATETIME(X) => CONVERT(DATETIME, CONVERT(VARCHAR(255), expression, 102), 102)
// Katmai: TRUNCATETIME(X) => CONVERT(DATETIME2, CONVERT(VARCHAR(255), expression, 102), 102)
// PreKatmai: TRUNCATETIME(X) => DATEADD(d, DATEDIFF(d, 0, expression), 0)
// Katmai: TRUNCATETIME(X) => CAST(CAST(expression AS DATE) as DATETIME2)
divega marked this conversation as resolved.
Show resolved Hide resolved
// TruncateTime(DateTimeOffset X)
// TRUNCATETIME(X) => CONVERT(datetimeoffset, CONVERT(VARCHAR(255), expression, 102)
// + ' 00:00:00 ' + Right(convert(varchar(255), @arg, 121), 6), 102)
// Katmai only: TRUNCATETIME(X) => TODATETIMEOFFSET(CAST(expression AS DATE), DATEPART(tz, expression)))
// </summary>
private static ISqlFragment HandleCanonicalFunctionTruncateTime(SqlGenerator sqlgen, DbFunctionExpression e)
{
//The type that we need to return is based on the argument type.
string typeName = null;
var isDateTimeOffset = false;

var typeKind = e.Arguments[0].ResultType.GetPrimitiveTypeKind();
var isDateTimeOffset = (typeKind == PrimitiveTypeKind.DateTimeOffset);

if (typeKind == PrimitiveTypeKind.DateTime)
{
typeName = sqlgen.IsPreKatmai ? "datetime" : "datetime2";
}
else if (typeKind == PrimitiveTypeKind.DateTimeOffset)
{
typeName = "datetimeoffset";
isDateTimeOffset = true;
}
else
if (!isDateTimeOffset && typeKind != PrimitiveTypeKind.DateTime)
{
Debug.Assert(true, "Unexpected type to TruncateTime" + typeKind.ToString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, Debug.Assert(true, ...) is also a GNDN. Will be properly fixed as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that.

}


var result = new SqlBuilder();
result.Append("convert (");
result.Append(typeName);
result.Append(", convert(varchar(255), ");
result.Append(e.Arguments[0].Accept(sqlgen));
result.Append(", 102) ");

if (isDateTimeOffset)
if (sqlgen.IsPreKatmai)
{
if (isDateTimeOffset)
{
throw new NotSupportedException(Strings.SqlGen_CanonicalFunctionNotSupportedPriorSql10(e.Function.Name));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

result.Append("dateadd(d, datediff(d, 0, ");
result.Append(e.Arguments[0].Accept(sqlgen));
result.Append("), 0)");
}
else
{
result.Append("+ ' 00:00:00 ' + Right(convert(varchar(255), ");
if (!isDateTimeOffset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer code clarity: I don't think the few lines that are common are worth the entanglement of the two translations.

{
result.Append("cast(");
}
else
{
result.Append("todatetimeoffset(");
}

result.Append("cast(");
result.Append(e.Arguments[0].Accept(sqlgen));
result.Append(", 121), 6) ");
result.Append(" as date)");

if (!isDateTimeOffset)
{
result.Append(" as datetime2)");
}
else
{
result.Append(", datepart(tz, ");
result.Append(e.Arguments[0].Accept(sqlgen));
result.Append("))");
}
}

result.Append(", 102)");
return result;
}

Expand Down
12 changes: 3 additions & 9 deletions src/EntityFramework.SqlServerCompact/SqlGen/SqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3148,13 +3148,10 @@ private static void AppendConvertToNVarchar(SqlGenerator sqlgen, SqlBuilder resu

// <summary>
// TruncateTime(DateTime X)
// TRUNCATETIME(X) => CONVERT(DATETIME, CONVERT(VARCHAR(255), expression, 102), 102)
// TRUNCATETIME(X) => DATEADD(d, DATEDIFF(d, 0, expression), 0)
// </summary>
private static ISqlFragment HandleCanonicalFunctionTruncateTime(SqlGenerator sqlgen, DbFunctionExpression e)
{
//The type that we need to return is based on the argument type.
string typeName = "datetime";

PrimitiveTypeKind typeKind;
TypeHelpers.TryGetPrimitiveTypeKind(e.ResultType, out typeKind);

Expand All @@ -3164,13 +3161,10 @@ private static ISqlFragment HandleCanonicalFunctionTruncateTime(SqlGenerator sql
}

var result = new SqlBuilder();
result.Append("convert (");
result.Append(typeName);
result.Append(", convert(nvarchar(255), ");
result.Append("dateadd(d, datediff(d, 0, ");
result.Append(e.Arguments[0].Accept(sqlgen));
result.Append(", 102) ");
result.Append("), 0)");

result.Append(", 102)");
return result;
}

Expand Down
2 changes: 1 addition & 1 deletion test/EntityFramework/FunctionalTests/Query/SqlCeCanonicalFunctionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Assert.Equal(new DateTime(1990, 2, 2, 0, 0, 0), query.First());
}
}
Expand Down