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

#17170 - DATEDIFF Week Implementation #17226

Closed
wants to merge 2 commits into from
Closed

#17170 - DATEDIFF Week Implementation #17226

wants to merge 2 commits into from

Conversation

ralmsdeveloper
Copy link
Contributor

Resolve #17170

btecu
btecu reviewed Aug 18, 2019
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated
/// <param name="_">The DbFunctions instance.</param>
/// <param name="startDate">Starting date for the calculation.</param>
/// <param name="endDate">Ending date for the calculation.</param>
/// <returns>Number of year boundaries crossed between the dates.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

Number of week not year

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I really missed it.

btecu
btecu reviewed Aug 18, 2019
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated
.GetWeekOfYear(
startDate,
CalendarWeekRule.FirstFullWeek,
DayOfWeek.Monday);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it matters but in most places the first day of the week is Sunday

btecu
btecu reviewed Aug 18, 2019
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated
/// <param name="_">The DbFunctions instance.</param>
/// <param name="startDate">Starting date for the calculation.</param>
/// <param name="endDate">Ending date for the calculation.</param>
/// <returns>Number of year boundaries crossed between the dates.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

Number of weeks instead of year

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 18, 2019

DATEDIFF always uses Sunday as the first day of the week to ensure the function operates in a deterministic way.

@ralmsdeveloper
Copy link
Contributor Author

kkk, I climbed my last test, was testing some possibilities.

I was sure it was sunday.

btecu
btecu reviewed Aug 18, 2019
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated
@@ -880,6 +881,97 @@ private static bool ContainsCore(string propertyName, string searchCondition, in
? (int?)DateDiffNanosecond(_, startTimeSpan.Value, endTimeSpan.Value)
: null;

/// <summary>
/// Counts the number of year boundaries crossed between the startDate and endDate.
Copy link
Contributor

Choose a reason for hiding this comment

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

year -> week

@ralmsdeveloper ralmsdeveloper force-pushed the feature/datediffweek branch 2 times, most recently from 8a98a15 to badf500 Compare August 18, 2019 18:38
@WeihanLi
Copy link
Contributor

@ralmsdeveloper thanks for your effort, could there's a parameter to specific the the first day of week, the Monday is the first day of the week for China in most cases

@ajcvickers
Copy link
Member

@smitpatel Note that this isn't a priority; we're not taking this for 3.0.

@smitpatel
Copy link
Member

could there's a parameter to specific the the first day of week, the Monday is the first day of the week for China in most cases

No. That is not possible. According to SqlServer documentation
SET DATEFIRST is used to configure first day of the week. DATEDIFF function has nothing to do with it. And as @ErikEJ mentioned above, it does not have any effect on DATEDIFF.
If you add such parameter then there is no SQL translation for it.

@WeihanLi
Copy link
Contributor

WeihanLi commented Aug 20, 2019 •
edited
Loading

could there's a parameter to specific the the first day of week, the Monday is the first day of the week for China in most cases

No. That is not possible. According to SqlServer documentation
SET DATEFIRST is used to configure first day of the week. DATEDIFF function has nothing to do with it. And as @ErikEJ mentioned above, it does not have any effect on DATEDIFF.
If you add such parameter then there is no SQL translation for it.

@shimingsg thanks for your info, got it from the docs here also

Specifying SET DATEFIRST has no effect on DATEDIFF. DATEDIFF always uses Sunday as the first day of the week to ensure the function operates in a deterministic way.

smitpatel
smitpatel requested changes Sep 4, 2019
View reviewed changes
Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Need more testing as requested.

@ralmsdeveloper
Copy link
Contributor Author

@ralmsdeveloper agradecimentos para seu esforço, poderia lá é um parâmetro a específico o o primeiro dia da semana, a segunda-feira é o primeiro dia da semana para China em a maioria de casos

We can't do this, sql server uses deterministically Sunday, that means the translation will not match the SQL Server result.
Besides the DATEDIFF function does not have the parameter to define what would be the first day of the week.

https://docs.microsoft.com/pt-br/sql/t-sql/functions/datediff-transact-sql?view=sql-server-2017

@WeihanLi
But if you really have this need you can build a custom function for yourself.

@ralmsdeveloper
Copy link
Contributor Author

Need more testing as requested.

Done!

@smitpatel smitpatel mentioned this pull request Sep 9, 2019
Merged
@smitpatel
Copy link
Member

Merging to release/3.1 in #17726

@ralmsdeveloper - Thank you for contribution.

@smitpatel smitpatel closed this Sep 9, 2019
@ralmsdeveloper ralmsdeveloper deleted the feature/datediffweek branch September 10, 2019 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bricelam bricelam bricelam left review comments

@btecu btecu btecu left review comments

@smitpatel smitpatel smitpatel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ralmsdeveloper @ErikEJ @WeihanLi @ajcvickers @smitpatel @bricelam @btecu