Translate 'case when X is null...' to 'coalesce(X,...)' by bgurmendi · Pull Request #93 · npgsql/EntityFramework6.Npgsql

bgurmendi · 2018-04-30T09:22:25Z

This changes solves most cases of #62 #60 #87 #90

Emill

Nice solution! Just a few comments...

Emill · 2018-04-30T09:46:32Z

src/EntityFramework6.Npgsql/SqlGenerators/SqlBaseGenerator.cs

+
+                if (is_null.Argument.Equals(expression.Else))
+                {
+                    LiteralExpression coalesceExpression = new LiteralExpression(" COALESCE( ");


Aren't we missing coalesceExpression.Append(expression.Else.Accept(this)); here?

You also right! I have deleted this line on some cleanup.

Emill · 2018-04-30T09:47:21Z

src/EntityFramework6.Npgsql/SqlGenerators/SqlBaseGenerator.cs

                        throw new NotSupportedException("cast type name argument must be a constant expression.");

                    return new CastExpression(args[0].Accept(this), typeNameExpression.Value.ToString());
+                }else if (functionName == "coalesce")


How can this code be triggered? Isn't it enough with the code above?

You are right! I have made a mistake cleaning code, this fragment is for general case to coalesce translation in other pull request #94

rwasef1830 · 2018-05-02T15:31:42Z

src/EntityFramework6.Npgsql/SqlGenerators/SqlBaseGenerator.cs

+            {
+                var is_null = expression.When[0] as DbIsNullExpression;
+                if (is_null.Argument.Equals(expression.Else))
+                {


Minor nitpick: code naming conventions, is_null should be isNull. To be more consistent with code below, it should be isNullExpression.

rwasef1830 · 2018-05-02T15:42:14Z

@bgurmendi Also, add a unit test that tests the change this code makes by asserting the SQL generated, and a unit test that tests that the string scenarios in the unit tests linked are solved (I see there are some unit tests in the other pull request, maybe you can adapt them and make them part for this one). If the solution can be with only 19 lines of code, it is preferable to keep it to this pull only and close the other one.

Thanks a lot for your valuable contribution!

bgurmendi · 2018-05-04T10:51:49Z

Thanks for the quick review.

Changes made:

Unit test were added.
Changed variable names

rwasef1830

I'm happy you found a solution for those obscure type unknown issues, I'm ready to merge this as soon as code formatting and naming issues are fixed. Thanks a lot!

rwasef1830 · 2018-05-04T17:00:51Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+            {
+                context.Database.Log = Console.Out.WriteLine;
+
+                context.Blogs.Add( new Blog { Name = "Hello" });


Nitpick: Extra space before new keyword.

rwasef1830 · 2018-05-04T17:02:01Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+                var blogTitle = query.First();
+                Assert.That(blogTitle, Is.EqualTo("string_value_postfix"));
+                Console.WriteLine(query.ToString());
+                StringAssert.AreEqualIgnoringCase("SELECT  COALESCE( @p__linq__0,E'')  || E'_postfix' AS \"C1\" FROM \"dbo\".\"Blogs\" AS \"Extent1\"", query.ToString());


Readability: This line is too long and is hard to read for other developers, break it at sensible points into 2 or more strings concatenated with "+" so that the length of each line is as near in length to surrounding code. Also there is extra spaces around COALESCE statement. You don't have to add these spaces in the expression. The parsing system automatically handles that.

rwasef1830 · 2018-05-04T17:03:08Z

src/EntityFramework6.Npgsql/SqlGenerators/SqlBaseGenerator.cs

+                var isNullExpression = expression.When[0] as DbIsNullExpression;
+                if (isNullExpression.Argument.Equals(expression.Else))
+                {
+                    LiteralExpression coalesceExpression = new LiteralExpression(" COALESCE( ");


No need for extra space before and after the coalesce statement.

rwasef1830 · 2018-05-04T17:03:24Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+                Assert.That(blog_title, Is.EqualTo("string_value_postfix"));
+
+                Console.WriteLine(query.ToString());
+                StringAssert.AreEqualIgnoringCase("SELECT  CASE  WHEN ( COALESCE( @p__linq__0,E'default_value')  IS NULL) THEN (E'') WHEN (@p__linq__0 IS NULL) THEN (E'default_value') ELSE (@p__linq__0) END  || E'_postfix' AS \"C1\" FROM \"dbo\".\"Blogs\" AS \"Extent1\"", query.ToString());


Readability: This line is too long and is hard to read for other developers, break it at sensible points into 2 or more strings concatenated with "+" so that the length of each line is as near in length to surrounding code.

rwasef1830 · 2018-05-04T17:03:33Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+                Assert.That(blog_title, Is.EqualTo("string_value1_postfix"));
+
+                Console.WriteLine(query.ToString());
+                StringAssert.AreEqualIgnoringCase("SELECT  CASE  WHEN ( COALESCE( @p__linq__0, COALESCE( @p__linq__1,@p__linq__2) )  IS NULL) THEN (E'') WHEN (@p__linq__0 IS NULL) THEN ( COALESCE( @p__linq__1,@p__linq__2) ) ELSE (@p__linq__0) END  || E'_postfix' AS \"C1\" FROM \"dbo\".\"Blogs\" AS \"Extent1\"", query.ToString());


Readability: This line is too long and is hard to read for other developers, break it at sensible points into 2 or more strings concatenated with "+" so that the length of each line is as near in length to surrounding code.

rwasef1830 · 2018-05-04T17:04:50Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

        }
+
+        [Test]
+        public void Test_issue_60_and_62()


This test needs a better name, maybe: Test_string_type_inference_in_coalesce_statements

rwasef1830 · 2018-05-04T17:05:25Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+        }
+
+        [Test]
+        public void TestNullPropagation_1()


Test needs a better name, maybe: Test_string_null_propagation

rwasef1830 · 2018-05-04T17:05:50Z

test/EntityFramework6.Npgsql.Tests/EntityFrameworkBasicTests.cs

+        }
+
+        [Test]
+        public void TestNullPropagation_2()


Test needs a better name, maybe: Test_string_multiple_null_propagation

rwasef1830 · 2018-06-01T12:07:44Z

@bgurmendi Are you still interested on this ? If you are busy, I don't mind making the needed changes and merging this.

bgurmendi · 2018-06-01T13:32:06Z

Yes please make the changes we are busy with a release to production. Thanks! El vie., 1 jun. 2018 14:07, Raif Atef <notifications@github.com> escribió:

…

@bgurmendi <https://github.com/bgurmendi> Are you still interested on this ? If you are busy, I don't mind making the needed changes and merging this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABRCCe6M-H9KgUo9LzpY1t0V5_KklXtrks5t4S6RgaJpZM4TsUaU> .

rwasef1830 · 2018-06-01T15:09:47Z

Merged via 11f1cbd

bgurmendi · 2018-06-01T15:32:50Z

@rwasef1830 thanks!!! Any estimation for release of this changes on Nuget?

Anyway thanks for the great work on EntityFramework6.Npgsql

rwasef1830 · 2018-06-01T15:34:14Z

@bgurmendi I don't have a way to make releases on NuGet. I am just implementing a few missing full text search functions and then I will ask @roji to make a release.

roji · 2018-06-01T16:28:08Z

@rwasef1830 let me know when you're ready and we'll release a version.

I propose we also update the dependency for Npgsql 4.0 and bump the EF6 provider version 4.0 as well...

rwasef1830 · 2018-06-01T20:38:29Z

@roji I committed and pushed the missing tsquery_phrase function.

I suggest making a release as-is to fix annoying "unknown type" errors for people using Npgsql 3.x and then update dependencies and make another release referencing Npgsql 4.0

I tried to update the Npgsql dependency to 4.0 but there are compile errors related to Postgis types.

roji · 2018-06-01T20:44:08Z

I suggest making a release as-is to fix annoying "unknown type" errors for people using Npgsql 3.x and then update dependencies and make another release referencing Npgsql 4.0

Makes sense, I'll do that this weekend.

I tried to update the Npgsql dependency to 4.0 but there are compile errors related to Postgis types.

Ah of course, we moves the Postgis types out to the Npgsql.LegacyPostgis plugin. I think the EF6 provider will have to simply reference it.

Emill suggested changes Apr 30, 2018

View reviewed changes

bgurmendi closed this Apr 30, 2018

bgurmendi reopened this Apr 30, 2018

bgurmendi mentioned this pull request May 2, 2018

Translate nested CASE to simpler COALESCE #94

Open

bgurmendi force-pushed the fix_Error_sql_42P18_issue_62_and_60 branch 2 times, most recently from c2304c6 to e32babe Compare May 2, 2018 09:31

rwasef1830 reviewed May 2, 2018

View reviewed changes

Translate 'case when X is null...' to 'coalesce(X,...)'

9522c2e

bgurmendi force-pushed the fix_Error_sql_42P18_issue_62_and_60 branch from e32babe to 9522c2e Compare May 3, 2018 15:10

rwasef1830 suggested changes May 4, 2018

View reviewed changes

rwasef1830 closed this Jun 1, 2018

rwasef1830 mentioned this pull request Jul 16, 2018

Lambda queries with string concatenation throws Exception with message "42P18: could not determine data type of parameter $1" #62

Closed

Conversation

bgurmendi commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwasef1830 commented May 2, 2018

Uh oh!

bgurmendi commented May 4, 2018

Uh oh!

rwasef1830 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwasef1830 commented Jun 1, 2018

Uh oh!

bgurmendi commented Jun 1, 2018 via email

Uh oh!

rwasef1830 commented Jun 1, 2018

Uh oh!

bgurmendi commented Jun 1, 2018

Uh oh!

rwasef1830 commented Jun 1, 2018

Uh oh!

roji commented Jun 1, 2018

Uh oh!

rwasef1830 commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented Jun 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bgurmendi commented Apr 30, 2018 •

edited

Loading

rwasef1830 commented Jun 1, 2018 •

edited

Loading