-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add function mapping for Ticks to sqlserver #35465
base: main
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree |
@z505985755 Are you going to fix the tests? |
sqlExpressionFactory.Divide( | ||
sqlExpressionFactory.Function( | ||
"DATEPART", | ||
arguments: new[] { sqlExpressionFactory.Fragment("nanosecond"), instance! }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am noting that this duplicate instance
twice in the translation; if that's a complicated expression (e.g. with a subquery), that's bad for performance - for those cases, we tend to do the translation only if we know that's it's a simple column/parameter, rather than a complex expression. I also saw a brief discussion over multiple translation possibilities (possibly some not supported in older SQL Server), I'd lean towards one that doesn't duplicate expressions - even if it's not supported across all versions/variants - rather than one that's supported everything but duplicates.
(regardless, this is missing test coverage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If use datediff_big to calculate the microsecond difference and then multiply it by 10, may lose one digit of precision because the datetime2 data type supports up to 7 decimal places. If use datediff_big to calculate the nanosecond difference and then divide it by 100, it will cause an overflow.
So, I can't think of a solution without using complex expressions.What are you suggesting?
yes,I ran some tests |
The CI failures seem related to the changes. Those need to fixed. |
I noticed this error when executing the test script. It seems that the assertion sql is inconsistent with the generated sql due to the original lack of support for datetime_Ticks. This is also consistent with a bug in CI. How should this be fixed? |
You need to adjust baselines. It's fine that the SQL produced is now different. |
Set |
##35436