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

Cosmos: Add translator for Substring method which map to SUBSTRING built-in functions #24284

Merged
merged 2 commits into from
Mar 1, 2021

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Feb 26, 2021

Add translator for string.Substring()

Issue #16143

Please review.
Thank you in advance

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks for all these translation contributions @Marusyk!

Can you please try enabling NorthwindWhereQueryCosmosTest.Where_string_substring as well? Note that there are other skipped tests for Trim/TrimStart/TrimEnd in NorthwindFunctionsQueryCosmosTest, can you please check those too?

Finally, if you intending to submit more string translations, may I suggest submitting them in a single PR rather than a PR-per-translation? This simplifies the review process and the general project history.

@Marusyk
Copy link
Member Author

Marusyk commented Feb 26, 2021

Thanks for all these translation contributions @Marusyk!

Not at all! I need this for my work

Finally, if you intending to submit more string translations, may I suggest submitting them in a single PR rather than a PR-per-translation? This simplifies the review process and the general project history.

@roji, I will but, I do it in my spare time, so, when I have time to add more than one translation, then I will do it within one PR. But when I have time to add only one translation, I don't see a problem in doing it within different PRs.
Or do you suggest not submit more PRs until I make all the string translations?

Translations are separate parts of the functionality that are not dependent on each other in most cases. Could you please explain how this can affect the general history of the project?

if you intending to submit more string translations, may I suggest submitting them in a single PR

What if I want to submit only a few of them? Only one of them? What if I want to submit one today and the other in the next week? Should I wait 2 weeks and then merge conflicts in that case?
Please explain the problem in a PR-per-translation

@Marusyk
Copy link
Member Author

Marusyk commented Feb 26, 2021

Note that there are other skipped tests for Trim/TrimStart/TrimEnd in NorthwindFunctionsQueryCosmosTest, can you please check those too?

All of them are not relevant because Cosmos DB TRIM/LTRIM/RTRIM does not take arguments (the same as SQL Server)

if (_trimStartMethodInfoWithoutArgs?.Equals(method) == true
|| (_trimStartMethodInfoWithCharArrayArg.Equals(method)
// Cosmos DB LTRIM does not take arguments
&& ((arguments[0] as SqlConstantExpression)?.Value as Array)?.Length == 0))
{
return TranslateSystemFunction("LTRIM", method.ReturnType, instance);
}
if (_trimEndMethodInfoWithoutArgs?.Equals(method) == true
|| (_trimEndMethodInfoWithCharArrayArg.Equals(method)
// Cosmos DB RTRIM does not take arguments
&& ((arguments[0] as SqlConstantExpression)?.Value as Array)?.Length == 0))
{
return TranslateSystemFunction("RTRIM", method.ReturnType, instance);
}
if (_trimMethodInfoWithoutArgs?.Equals(method) == true
|| (_trimMethodInfoWithCharArrayArg.Equals(method)
// Cosmos DB TRIM does not take arguments
&& ((arguments[0] as SqlConstantExpression)?.Value as Array)?.Length == 0))
{
return TranslateSystemFunction("TRIM", method.ReturnType, instance);
}

image

@ajcvickers ajcvickers assigned roji and maumar and unassigned roji Feb 26, 2021
@maumar maumar merged commit 8936a65 into dotnet:main Mar 1, 2021
@maumar
Copy link
Contributor

maumar commented Mar 1, 2021

@Marusyk thank you for the contribution!

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