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

Add function mapping for Ticks to sqlserver #35465

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

z505985755
Copy link

@z505985755 z505985755 requested a review from a team as a code owner January 14, 2025 06:05
@z505985755
Copy link
Author

@dotnet-policy-service agree

@dotnet-policy-service agree

@cincuranet
Copy link
Contributor

@z505985755 Are you going to fix the tests?

sqlExpressionFactory.Divide(
sqlExpressionFactory.Function(
"DATEPART",
arguments: new[] { sqlExpressionFactory.Fragment("nanosecond"), instance! },
Copy link
Member

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)

Copy link
Author

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?

@z505985755
Copy link
Author

@z505985755 Are you going to fix the tests?

yes,I ran some tests

@SamMonoRT
Copy link
Member

@z505985755 Are you going to fix the tests?

yes,I ran some tests

The CI failures seem related to the changes. Those need to fixed.

@z505985755
Copy link
Author

@z505985755 Are you going to fix the tests?

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?
image
image

@cincuranet
Copy link
Contributor

You need to adjust baselines. It's fine that the SQL produced is now different.

@cincuranet cincuranet marked this pull request as draft February 4, 2025 11:24
@AndriySvyryd
Copy link
Member

Set EF_TEST_REWRITE_BASELINES env var to TRUE before running the tests to fix them automatically

@z505985755 z505985755 marked this pull request as ready for review February 8, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants