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

#18132 Added regression tests for concatenating int and strings together #19948

Merged
merged 1 commit into from
Feb 20, 2020
Merged

#18132 Added regression tests for concatenating int and strings together #19948

merged 1 commit into from
Feb 20, 2020

Conversation

Muppets
Copy link
Contributor

@Muppets Muppets commented Feb 16, 2020

Summary of the changes

  • Added regression test to verify concatenating int and strings together doesn't fail with "Null TypeMapping in Sql Tree"
  • I rolled the code back to the 3.1 release branch and verified the tests failed on that branch.

Fixes #18132

Muppets
Muppets commented Feb 16, 2020
View reviewed changes
test/EFCore.Sqlite.FunctionalTests/Query/NorthwindMiscellaneousQuerySqliteTest.cs Outdated
@@ -242,6 +242,17 @@ public override Task Complex_nested_query_doesnt_try_binding_to_grandparent_when

public override Task AsQueryable_in_query_server_evals(bool async) => null;

public override Task String_int_concat(bool async) => null;
Copy link
Contributor Author

@Muppets Muppets Feb 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I had to null off this test in SQLite as it fails the result verification. The query produces <orderId> but the expected results are <orderid><customerid>. The other frameworks seem to handle this fine. Is this expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a bug in sqlite - whenever possible we should be returning results matching Ling to Objects

Copy link
Contributor

Choose a reason for hiding this comment

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

we should be using || operator for string concat in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

filed #19990

Copy link
Contributor

@maumar maumar Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@Muppets - we looked at the issue briefly with @smitpatel and it's potentially quite a complicated problem to fix properly. So for now, you can just skip the test on sqlite using 19990 as reason (rather than null it out)

@maumar
Copy link
Contributor

maumar commented Feb 20, 2020

@Muppets some other cases you can consider: one argument being a parameter, also some tests with reversed order (string + int rather than int + string)

@Muppets Muppets force-pushed the #18132-Can't-concatenate-string-inside-query branch from 259d299 to 5ce1b7d Compare February 20, 2020 22:11
@smitpatel
Copy link
Member

@dotnet/aspnet-build - Is running helix test for community PRs not allowed?
Error message

  Starting Azure Pipelines Test Run Ubuntu.1804.Amd64.Open
/home/vsts/.nuget/packages/microsoft.dotnet.helix.sdk/5.0.0-beta.20116.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(16,5): error : Request to https://dev.azure.com/dnceng/public/_apis/test/runs?api-version=5.0-preview.2 returned failed status 401 Unauthorized [/home/vsts/work/1/s/eng/helix.proj]
/home/vsts/.nuget/packages/microsoft.dotnet.helix.sdk/5.0.0-beta.20116.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(16,5): error :  [/home/vsts/work/1/s/eng/helix.proj]
/home/vsts/.nuget/packages/microsoft.dotnet.helix.sdk/5.0.0-beta.20116.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(16,5): error :  [/home/vsts/work/1/s/eng/helix.proj]

@smitpatel smitpatel merged commit 8f73092 into dotnet:master Feb 20, 2020
@maumar
Copy link
Contributor

maumar commented Feb 20, 2020

@Muppets thanks for the contribution!

@Muppets Muppets deleted the #18132-Can't-concatenate-string-inside-query branch February 22, 2020 02:01
@ajcvickers ajcvickers mentioned this pull request Feb 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maumar maumar maumar left review comments

Assignees

@maumar maumar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't concatenate string inside query
3 participants
@Muppets @maumar @smitpatel