Translate x.parse #28337

MaRK0960 · 2022-06-28T18:38:34Z

I've read the guidelines for contributing and seen the walkthrough
I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
The code builds and tests pass locally (also verified by our automated build checks)
Commit messages follow this format:

        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber

Tests for the changes have been added (for bug fixes / features)
Code follows the same patterns and style as existing code in this repo

dnfadmin · 2022-06-28T18:38:49Z

All CLA requirements met.

src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindFunctionsQueryCosmosTest.cs

src/EFCore.SqlServer/Query/Internal/SqlServerParseTranslator.cs

smitpatel · 2022-06-29T19:01:22Z

Can we also get tests which converts from actual string values. Also some negative tests where the string is not exactly parse-able and we verify database behavior.

Added tests against overflow. Added tests against bad format.

roji

Thanks for the changes. This looks almost ready, see below for some nits and test cleanup.

src/EFCore.SqlServer/Query/Internal/SqlServerParseTranslator.cs

roji · 2022-07-09T08:04:34Z

test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs

+    {
+        await AssertQuery(
+            async,
+            ss => ss.Set<Order>().Where(o => o.CustomerID == "ALFKI")


Merge the two Where clauses (here and below). Also consider removing the check for CustomerID - is there any reason to restrict to those rows only?

Some rows are not parse-able, making them not suitable for this test.

roji · 2022-07-09T08:05:16Z

test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs

+                .Where(o => byte.Parse(Convert.ToString(o.OrderID % 1)) >= 0),
+            entryCount: 6);
+
+        await AssertQuery(


Any reason for the 2nd test query? I can see this below as well - the point here generally isn't to check that the database operation works (we don't test the database, only EF), so I'm not sure what this one checks beyond the above.

Unless the extra queries add coverage for EF specifically, I think it's better to just do one query per test.

test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs

nameof instead of string. Merge the Where clauses.

MaRK0960 · 2022-08-03T19:47:35Z

@roji Is there anything else?

tmricardo7 · 2022-11-17T03:07:08Z

any updates on this @roji ?

roji · 2022-11-17T08:44:51Z

@MaRK0960 sorry for not giving this any attention - the 7.0 release (and resulting bugfixes) are have taken up most of my time. I'll do my best to look at this again in the next couple of weeks.

roji

@MaRK0960 apologies for letting this sit for so long without reviewing. Aside from one important note (the one about type inference), I think this is more or less ready; I'm happy to also take over this (as we haven't responded for so long), let me know what you want to do.

roji · 2023-04-24T15:28:58Z

test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs

+           async,
+           ss => ss.Set<Customer>()
+                   .Where(c => c.CustomerID == "ALFKI")
+                   .Select(c => byte.Parse(c.CustomerID)));


It's never a good idea to put the tested invocation in the top-most select, because providers that don't have a translation for it will go into client evaluation. Ideally, these tests would simply have Parse in the Where clause instead.

roji · 2023-04-24T15:29:43Z

test/EFCore.InMemory.FunctionalTests/Query/NorthwindFunctionsQueryInMemoryTest.cs

@@ -14,4 +14,56 @@ public class NorthwindFunctionsQueryInMemoryTest : NorthwindFunctionsQueryTestBa
    {
        //TestLoggerFactory.TestOutputHelper = testOutputHelper;
    }
+
+    public override Task Byte_Parse_Non_Numeric_Bad_Format(bool async)


It's not great to force all providers to override and catch an exception... But on the other hand it's true that the exception type itself varies from provider to provider. It may be enough to simply verify that an exception is caught in the base implementation without asserting which kind it is.

There is a test in SqlServer that checks if all tests are overridden:

efcore/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs

Lines 26 to 28 in 3f10651

[ConditionalFact]

public virtual void Check_all_tests_overridden()

=> TestHelpers.AssertAllMethodsOverridden(GetType());

And if the assertion is done in the base class, we can't override it.

roji · 2023-04-24T15:30:58Z

src/EFCore.SqlServer/Query/Internal/SqlServerParseTranslator.cs

+        IDiagnosticsLogger<DbLoggerCategory.Query> logger)
+        => SupportedMethods.Contains(method)
+            ? _sqlExpressionFactory.Function(
+                "CONVERT",


There's a subtle problem here, where the type mapping coming out of the conversion is fixed and depends only on the CLR type; this doesn't allow for type inference to apply a type mapping from the other side of e.g. a comarison.

For example, let's assume you do int.Parse(decimalParam) == b.SomeDecimalColumn, where SomeColumn has a type mapping of decimal(5,2). Your translation of int.Parse would convert decimalParam to decimal(18,2), because that's the fixed value in the table above; but it should allow the column type mapping to be inferred from the other side instead.

The right way to do this would be to simply use _sqlExpressionFactory.Convert(), specifying only the CLR type and not the type mapping. This produces CAST(x AS y) instead of CONVERT, and since the type mapping is null, it can be inferred from the other side.

MaRK0960 · 2023-04-25T12:22:40Z

No problem, I will continue working on it.

… will be from client.

roji · 2023-04-25T14:40:32Z

@MaRK0960 ok great, let me know if you run out of time or something, otherwise you can ask for another review after you've made changes etc.

Saibamen · 2023-05-26T07:56:13Z

@roji: ping

MaRK0960 added 3 commits June 27, 2022 21:46

Added tests against x.Parse(string)

93564ff

Added SqlServerParseTranslator

79b94e8

Add failing tests in Sqlite and Cosmos

92401eb

smitpatel reviewed Jun 28, 2022

View reviewed changes

src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs Outdated Show resolved Hide resolved

MaRK0960 commented Jun 28, 2022

View reviewed changes

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindFunctionsQueryCosmosTest.cs Show resolved Hide resolved

Order translators alphabetically

c758b48

smitpatel reviewed Jun 29, 2022

View reviewed changes

src/EFCore.SqlServer/Query/Internal/SqlServerParseTranslator.cs Outdated Show resolved Hide resolved

MaRK0960 added 2 commits June 29, 2022 21:43

Renamed TypeMapping to ClrTypeToStoreTypeMap

2acf362

Added more tests.

3976d4b

Added tests against overflow. Added tests against bad format.

ajcvickers assigned roji Jul 8, 2022

roji requested changes Jul 9, 2022

View reviewed changes

MaRK0960 added 2 commits July 11, 2022 13:05

Write cleaner code.

3a7e6a5

nameof instead of string. Merge the Where clauses.

Remove redundant tests.

3f10651

MaRK0960 requested a review from roji July 19, 2022 17:32

roji requested changes Apr 24, 2023

View reviewed changes

roji added the waiting-for-response label Apr 24, 2023

MaRK0960 added 2 commits April 25, 2023 14:55

Parse and Convert are not implemented in Sqlite, so exceptions thrown…

cf0f4ae

… will be from client.

The tested invocation should be in the Where clause

d9447cb

MaRK0960 and others added 2 commits May 4, 2023 21:05

Use type inference instead of fixed type

2706a05

Merge branch 'main' into translate-x.parse

bfad808

MaRK0960 requested a review from roji May 4, 2023 19:25

ajcvickers removed the waiting-for-response label Jan 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate x.parse #28337

Translate x.parse #28337

MaRK0960 commented Jun 28, 2022 •

edited

dnfadmin commented Jun 28, 2022 •

edited

smitpatel commented Jun 29, 2022

roji left a comment

roji Jul 9, 2022

MaRK0960 Jul 11, 2022

roji Jul 9, 2022

MaRK0960 commented Aug 3, 2022

tmricardo7 commented Nov 17, 2022

roji commented Nov 17, 2022

roji left a comment

roji Apr 24, 2023

roji Apr 24, 2023

MaRK0960 Apr 27, 2023

roji Apr 24, 2023

MaRK0960 commented Apr 25, 2023

roji commented Apr 25, 2023

Saibamen commented May 26, 2023

	[ConditionalFact]
	public virtual void Check_all_tests_overridden()
	=> TestHelpers.AssertAllMethodsOverridden(GetType());

Translate x.parse #28337

Are you sure you want to change the base?

Translate x.parse #28337

Conversation

MaRK0960 commented Jun 28, 2022 • edited

dnfadmin commented Jun 28, 2022 • edited

smitpatel commented Jun 29, 2022

roji left a comment

Choose a reason for hiding this comment

roji Jul 9, 2022

Choose a reason for hiding this comment

MaRK0960 Jul 11, 2022

Choose a reason for hiding this comment

roji Jul 9, 2022

Choose a reason for hiding this comment

MaRK0960 commented Aug 3, 2022

tmricardo7 commented Nov 17, 2022

roji commented Nov 17, 2022

roji left a comment

Choose a reason for hiding this comment

roji Apr 24, 2023

Choose a reason for hiding this comment

roji Apr 24, 2023

Choose a reason for hiding this comment

MaRK0960 Apr 27, 2023

Choose a reason for hiding this comment

roji Apr 24, 2023

Choose a reason for hiding this comment

MaRK0960 commented Apr 25, 2023

roji commented Apr 25, 2023

Saibamen commented May 26, 2023

MaRK0960 commented Jun 28, 2022 •

edited

dnfadmin commented Jun 28, 2022 •

edited