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

EF.Functions: Translating ISDATE to SQL Server #16471

Merged
merged 8 commits into from
Jul 15, 2019

Conversation

ralmsdeveloper
Copy link
Contributor

Implementation for #8488

As this function returns 0/1.
So I thought it would be better to make it Boolean.

https://docs.microsoft.com/en-us/sql/t-sql/functions/isdate-transact-sql?view=sql-server-2017

cc/ @divega @smitpatel

@ralmsdeveloper
Copy link
Contributor Author

@AndriySvyryd

Can you tell me why my tests broke?

I did not find anything, only messages CI.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Jul 7, 2019

Microsoft.EntityFrameworkCore.SqlServerDbFunctionsExtensions.IsDate[expression] is missing NotNull annotation

/// <param name="_">The DbFunctions instance.</param>
/// <param name="expression">Expression to validate</param>
/// <returns>true to valid and false not valid.</returns>
public static bool IsDate(
Copy link
Contributor

Choose a reason for hiding this comment

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

@divega - Should this return bool or int. SqlServer ISDATE returns int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: as I reported here: #16471 (comment).
It would be much nicer and semantic to use Boolean, since ISDATE returns only 0/1, but if we should follow exactly what SQLServer tells us, we should return an integer, but I would be happy if it was boolean already that makes more sense.

@smitpatel
Copy link
Contributor

Only waiting for @divega to decide on return type now.

@divega
Copy link
Contributor

divega commented Jul 14, 2019

It appears that functions like ISDATE and ISNUMERIC only return int for historical reasons. All other things being equal, bool seems a better type for the method in .NET, unless there are drawbacks with it.

@divega divega removed the blocked label Jul 14, 2019
@@ -556,6 +560,64 @@ public virtual void DateDiff_Nanosecond()
}
}

[ConditionalFact]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also have a positive test which recognize and returns true. In projection & where both.

@@ -879,5 +879,17 @@ private static bool ContainsCore(string propertyName, string searchCondition, in
=> (startTimeSpan.HasValue && endTimeSpan.HasValue)
? (int?)DateDiffNanosecond(_, startTimeSpan.Value, endTimeSpan.Value)
: null;

/// <summary>
/// Validate if a date is valid..
Copy link
Contributor

Choose a reason for hiding this comment

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

Validate if the given string is a valid date.

/// </summary>
/// <param name="_">The DbFunctions instance.</param>
/// <param name="expression">Expression to validate</param>
/// <returns>true to valid and false not valid.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

true for valid date and false otherwise.

@mburbea
Copy link

mburbea commented Jul 15, 2019

Sql server's ISDATE function is limited to formats supported by DateTime and will reject valid dates that can be converted to types introduced in sql server 2008 (datetime2,datetimeoffset,date and time). A datetime2(3) is basically a better datetime than the datetime object.

select priorTo1753=isDate('1000-01-01')
         ,highPrecision = isDate('2019-07-15T00:00:00.1234')

For my money, a better test might be to use the try_* of functions and test against null.

@ralmsdeveloper
Copy link
Contributor Author

@smitpatel I made the requested adjustments and also the rebase.

@smitpatel smitpatel merged commit f8ee688 into dotnet:master Jul 15, 2019
@smitpatel
Copy link
Contributor

@ralmsdeveloper - Thanks.

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.

6 participants