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 Math translators for Cosmos DB provider #24463

Merged
merged 5 commits into from
Jun 17, 2021

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Mar 21, 2021

Added Math for Cosmos:
ABS, CEILING, FLOOR, POWER, EXP, LOG10, LOG, SQRT, ACOS, ASIN, ATAN, ATN2, COS, SIN, TAN, SIGN, TRUNC, ROUND

Issue: #16143

Need your help with translation methods with converting.
Example:

.Where(od => Math.Tan(od.Discount) > 0)

here od.Discount is float. Math.Tan requires double. So, the following expression will be generated:

Tan(Convert([Microsoft.EntityFrameworkCore.Query.EntityShaperExpression].Discount, Double))

In this case the TryRemoveImplicitConvert will be called and return null

https://github.com/Marusyk/efcore/blob/e23cdc0128851a8760bbcd57696f4001ae3b3a60/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs#L685

Could you please suggest how to improve that?

@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

@maumar I'm poaching this.

@bricelam bricelam assigned bricelam and unassigned maumar May 4, 2021
@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

@Marusyk Interested in adding the MathF versions too? (like PR #24829) If not, I'll add them while merging...

@Marusyk
Copy link
Member Author

Marusyk commented May 4, 2021

@bricelam sure, I will add

@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

@maumar Looks like this still needs your expertise to answer some design questions.

@Marusyk
Copy link
Member Author

Marusyk commented May 10, 2021

I will add tests after merge of #18842

@bricelam bricelam removed their assignment Jun 7, 2021
@AndriySvyryd AndriySvyryd assigned smitpatel and unassigned maumar Jun 11, 2021
@smitpatel
Copy link
Member

@Marusyk - For now disable tests which are running into issue of convert node. Looking at the CosmosDb docs, seems like they only have double kind of numeric values. This would make any convert to double node in LINQ for numeric type no-op and translatable but I need to research more on this in order to update conversions.

@smitpatel
Copy link
Member

@Marusyk - 1 test is failing in SQL assertion (perhaps not getting translated now), can you also disable it and update the PR?

@Marusyk
Copy link
Member Author

Marusyk commented Jun 17, 2021

@smitpatel I've fixed it. Thanks

@smitpatel smitpatel merged commit 22bb1e4 into dotnet:main Jun 17, 2021
@smitpatel
Copy link
Member

@Marusyk - Thank you for contribution.

@Marusyk Marusyk deleted the rmarusyk/16143_math branch June 17, 2021 21:58
This pull request was closed.
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.

4 participants