Take 2 - Add support for user defined scalar functions #8507

pmiddleton · 2017-05-18T03:28:50Z

@anpete - Here is the new PR for adding UDF support.

This is a major refactor from the last PR.

Core UDF code moved to EFCore.Relational from EFCore
UDF metadata now stored in annotations on the model
UDF translation now ties into the existing function translation mechanism
Code first APIs now use the provider extension method pattern

anpete · 2017-05-18T19:27:32Z

@bricelam Looks like there are some minor changes to Migrations/Design here

anpete · 2017-05-18T19:28:24Z

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

+            Check.NotNull(annotations, nameof(annotations));
+            Check.NotNull(annotationPrefixes, nameof(annotationPrefixes));
+
+            foreach(var ignoreAnnotation in annotations.Where(a => !annotationPrefixes.All(pre => a.Name.StartsWith(pre) == false)).ToList())


Add braces even when single line block

anpete · 2017-05-18T19:36:19Z

@smitpatel Can you look at CF changes?

anpete · 2017-05-18T19:44:49Z

src/EFCore/Infrastructure/ModelSource.cs

@@ -79,6 +79,8 @@ public virtual IModel GetModel(DbContext context, IConventionSetBuilder conventi

            FindSets(modelBuilder, context);

+            CreateProviderModel(modelBuilder, context) ;


@ajcvickers Would a relational IModelCustomizer be a better option here?

anpete · 2017-05-18T19:46:50Z

src/EFCore.Relational/Metadata/DbFunction.cs

+    /// </summary>
+    public class DbFunction : IDbFunction, IMethodCallTranslator
+    {
+        private SortedDictionary<string, DbFunctionParameter> _parameters


readonly all fields where possible.

anpete · 2017-05-18T19:47:27Z

src/EFCore.Relational/Metadata/DbFunction.cs

+
+        private Type _returnType;
+        private readonly string _annotationName;
+        private readonly IModel _model;


Unused field.

anpete · 2017-05-18T19:47:43Z

src/EFCore.Relational/Metadata/DbFunction.cs

+
+            MethodInfo = dbFunctionMethodInfo;
+            ReturnType = MethodInfo.ReturnType;
+            Name = name ?? dbFunctionMethodInfo.Name;


name has NotNull attribute.

anpete · 2017-05-18T19:49:21Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+        {
+            foreach (var dbFunction in RelationalExtensions.For(model).DbFunctions)
+            {
+                if (String.IsNullOrEmpty(dbFunction.Name))


Use built-in type names.

anpete · 2017-05-18T19:50:20Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+                if (dbFunction.MethodInfo.IsStatic == false && dbFunction.MethodInfo.DeclaringType.GetTypeInfo().IsSubclassOf(typeof(DbContext)) == false)
+                    ShowError(CoreStrings.DbFunctionDbContextMethodMustBeStatic(dbFuncName));
+
+                var missingType = dbFunction.Parameters.Where(p => p.ParameterType == null).FirstOrDefault();


Can move predicate to FirstOrDefault and remove Where.

anpete · 2017-05-18T19:50:43Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+                if (missingType != null)
+                    ShowError(CoreStrings.DbFunctionParameterMissingType(dbFuncName, missingType.Name));
+
+                var arrayParam = dbFunction.Parameters.Where(p => p.ParameterType.GetTypeInfo().IsArray == true).FirstOrDefault();


Can move predicate to FirstOrDefault and remove Where.
redundant '== true'

anpete · 2017-05-18T19:51:56Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+                var paramIndexes = dbFunction.Parameters.Select(fp => fp.ParameterIndex).ToArray();
+                var dbFuncName = $"{dbFunction.MethodInfo.DeclaringType.Name}.{dbFunction.MethodInfo.Name}";
+
+                if (paramIndexes.Distinct().Count() != dbFunction.Parameters.Count())


nit: Use Parameters.Count property

anpete · 2017-05-18T19:52:53Z

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

+                if (Enumerable.Range(0, paramIndexes.Length).Except(paramIndexes).Any())
+                    ShowError(CoreStrings.DbFunctionNonContinuousIndex(dbFuncName));
+
+                if (dbFunction.MethodInfo.IsStatic == false && dbFunction.MethodInfo.DeclaringType.GetTypeInfo().IsSubclassOf(typeof(DbContext)) == false)


break long lines. We break these before '&&'

anpete · 2017-05-18T19:55:19Z

src/EFCore.Relational/Infrastructure/Internal/RelationalModelSource.cs

@@ -23,6 +23,18 @@ public RelationalModelSource([NotNull] ModelSourceDependencies dependencies)
        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
        ///     directly from your code. This API may change or be removed in future releases.
        /// </summary>
+        protected override void CreateProviderModel([NotNull] ModelBuilder modelBuilder, [NotNull] DbContext context)


Don't put NotNull annotations on overriding/implementing methods.

Should Check.NotNull still be performed in overriding methods for NotNull parameters?

Yeah, but Checks are only required for public components (not in .Internal namespace).

anpete · 2017-05-18T19:56:31Z

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalDbFunctionConvention.cs

+
+namespace Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal
+{
+    class RelationalDbFunctionConvention : IModelAnnotationSetConvention


anpete · 2017-05-18T19:58:28Z

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalDbFunctionConvention.cs

+    {
+        public Annotation Apply(InternalModelBuilder modelBuilder, string name, Annotation annotation, Annotation oldAnnotation)
+        {
+            if (name.StartsWith(RelationalFullAnnotationNames.Instance.DbFunctionPrefix))


Add StringComparison to StartsWith call

anpete · 2017-05-18T19:58:47Z

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalDbFunctionConvention.cs

+            { 
+                Check.NotNull(modelBuilder, nameof(modelBuilder));
+
+                var dbFunctionBuilder = new RelationalDbFunctionBuilder(annotation.Value as DbFunction) ;


annotation has CanBeNull

anpete · 2017-05-18T19:59:12Z

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalDbFunctionConvention.cs

+                                        ParameterInfo = pi,
+                                        DbFuncParamAttr = pi.GetCustomAttributes<DbFunctionParameterAttribute>().SingleOrDefault(),
+                                        IsParams = pi.CustomAttributes.Any(ca => ca.AttributeType == typeof(ParamArrayAttribute)),
+                                        ParameterType = pi.ParameterType


Redundant name

anpete · 2017-05-18T19:59:55Z

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalDbFunctionConvention.cs

+                }
+            }
+
+            return annotation ;


nit: formatting

What's the formatting nit here?

space before ';'

Yea my first TA in college had us do that to more easily see missing semis, back in the era before fancy ides and good compiler errors. 😄 Old habits die hard. I'll find a remove the rest of those.

pmiddleton · 2017-05-18T20:27:08Z

@anpete

I will get these all cleared up tonight and review the remaining files for similar patterns to get those cleaned as well.

anpete · 2017-05-18T20:40:28Z

@pmiddleton Not sure if you have R#, but it will help with a lot of these - based on our EFCore.sln.DotSettings file.

anpete · 2017-05-18T20:42:43Z

src/EFCore.Relational/Metadata/DbFunction.cs

+
+        private static string BuildAnnotationName(string annotationPrefix, MethodInfo methodInfo)
+        { 
+            StringBuilder sb = new StringBuilder($"{annotationPrefix}{methodInfo.Name}") ;


anpete · 2017-05-18T20:45:02Z

src/EFCore.Relational/Metadata/DbFunction.cs

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used 
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual bool HasTranslateCallback => TranslateCallback != null;


This needed?

Are you asking about just the HasTranslateCallback property or the entire TranslateCallback piece?

I added The TranslateCallback as a way for users to provider their own methodcall to SqlFunctionExpression translation in the cases where the fluent api isn't expressive enough. It gives the end user the most flexibility to perform a translation, but will it be too confusing to the average user to leave in there?

The HasTranslateCallback bool property itself can go away - I can move the != null checks to where they are actually used.

@pmiddleton +1 to moving the null checks.

anpete · 2017-05-18T20:45:41Z

src/EFCore.Relational/Metadata/DbFunction.cs

+        {
+            Check.NotNull(name, nameof(name));
+
+            DbFunctionParameter item;


out var all the things.

anpete · 2017-05-18T20:46:10Z

src/EFCore.Relational/Metadata/DbFunction.cs

+        {
+            Check.NotNull(name, nameof(name));
+
+            if (shiftParameters == true)


Redundant '== true'

anpete · 2017-05-18T20:46:23Z

src/EFCore.Relational/Metadata/DbFunction.cs

+
+                var parameters = Parameters;
+
+                for (int i = 0; i < parameters.Count; i++)


use var everywhere

anpete · 2017-05-18T20:46:36Z

src/EFCore.Relational/Metadata/DbFunction.cs

+            _parameters.Remove(name);
+        }
+
+        private bool IsValidReturnType(Type returnType)


Method can be static.

AndriySvyryd · 2017-06-06T18:57:46Z

src/EFCore.Relational/Metadata/Internal/InternalDbFunctionBuilder.cs

+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used 
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual InternalDbFunctionBuilder HasSchema([NotNull] string schema, ConfigurationSource configurationSource)


[CanBeNull]

smitpatel · 2017-06-07T04:54:40Z

@pmiddleton - Also if you are rebasing then rebase on rel/2.0.0-preview2 branch and change base in this PR. If its good and done in time, we may want to put this in preview2 release. It would be easier to rebase/merge into dev in either case.

AndriySvyryd

Metadata changes look good.
Rebase on origin/rel/2.0.0-preview2

pmiddleton · 2017-06-08T15:08:39Z

Ok I need a little git help. What command do I need to run to rebase onto origin/rel/2.0.0-preview2.

I pulled a tracked branch of rel/2.0.0-preview locally, but I'm not sure what to run to perform the rebase.

AndriySvyryd · 2017-06-08T15:18:18Z

@pmiddleton From your local branch:

git rebase -i origin/rel/2.0.0-preview2

Then remove all the commits that aren't yours from the rebase specification that pops up

pmiddleton · 2017-06-08T15:59:52Z

Ok I have rebased onto rel/2.0.0-preview2

AndriySvyryd · 2017-06-08T16:29:45Z

@pmiddleton There are still 4 commits that aren't yours. Run

git rebase -i origin/rel/2.0.0-preview2

again and make sure to remove them.

pmiddleton · 2017-06-08T18:31:13Z

@AndriySvyryd - I rebased and removed the other 4 commits.

pmiddleton · 2017-06-09T15:11:58Z

@AndriySvyryd - There are conflicts I need to resolve. At this point in the process can I do a pull --rebase to resolve or should I be doing a rebase -i, or does it not matter now?

anpete · 2017-06-09T19:11:07Z

Nice work @pmiddleton!

And thanks to @smitpatel, @AndriySvyryd and @bricelam for the review.

ajcvickers · 2017-06-09T19:13:59Z

Great to see this get in for preview2. Thanks to everybody!

pmiddleton · 2017-06-09T19:58:12Z

Thanks to everyone for the help getting this in!

Function quoting logic was changed (for now): all function names are quoted just like any other identifier (the default EF Core behavior is only to quote schema-less functions). This meant changing all expression translators to generate lower-case names (lower() instead of LOWER()), and also to add a hardcoded list of functions which EF Core generates in upper-case but which shouldn't get quoted (e.g. SUM()). See: * dotnet/efcore#8507 * dotnet/efcore#12044 * dotnet/efcore#12757 * dotnet/efcore#9558 * dotnet/efcore#9303

For now, only unquoted function names are supported (i.e. all lower- case, no special chars). Quoting logic for functions is quite complicated, see below issues for details. See: * dotnet/efcore#8507 * dotnet/efcore#12044 * dotnet/efcore#12757 * dotnet/efcore#9558 * dotnet/efcore#9303

dnfclas added the cla-already-signed label May 18, 2017

anpete self-assigned this May 18, 2017

anpete requested review from bricelam and smitpatel May 18, 2017 19:26

anpete reviewed May 18, 2017

View reviewed changes

AndriySvyryd reviewed Jun 6, 2017

View reviewed changes

pmiddleton force-pushed the udfTake2 branch 2 times, most recently from 9285710 to ee7e80d Compare June 7, 2017 04:43

pmiddleton force-pushed the udfTake2 branch from ee7e80d to 54ec075 Compare June 8, 2017 04:55

AndriySvyryd changed the base branch from dev to rel/2.0.0-preview2 June 8, 2017 05:01

AndriySvyryd approved these changes Jun 8, 2017

View reviewed changes

pmiddleton force-pushed the udfTake2 branch from 54ec075 to a81805a Compare June 8, 2017 14:41

pmiddleton force-pushed the udfTake2 branch 2 times, most recently from 1a737b8 to c802f23 Compare June 8, 2017 15:56

pmiddleton force-pushed the udfTake2 branch from c802f23 to 86379e4 Compare June 8, 2017 18:30

Add support for UDF Scalar functions

f4d74e6

pmiddleton force-pushed the udfTake2 branch from 86379e4 to f4d74e6 Compare June 9, 2017 15:42

anpete merged commit f4d74e6 into dotnet:rel/2.0.0-preview2 Jun 9, 2017

divega mentioned this pull request Jun 9, 2017

Query: Add support for User Defined Scalar Functions #7368

Closed

pmiddleton deleted the udfTake2 branch June 9, 2017 20:11

		@@ -79,6 +79,8 @@ public virtual IModel GetModel(DbContext context, IConventionSetBuilder conventi

		FindSets(modelBuilder, context);

		CreateProviderModel(modelBuilder, context) ;


		var parameters = Parameters;

		for (int i = 0; i < parameters.Count; i++)

Take 2 - Add support for user defined scalar functions #8507

Take 2 - Add support for user defined scalar functions #8507

Conversation

pmiddleton commented May 18, 2017

anpete commented May 18, 2017

Choose a reason for hiding this comment

anpete commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmiddleton commented May 18, 2017

anpete commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Jun 7, 2017

AndriySvyryd left a comment

Choose a reason for hiding this comment

pmiddleton commented Jun 8, 2017

AndriySvyryd commented Jun 8, 2017 • edited Loading

pmiddleton commented Jun 8, 2017

AndriySvyryd commented Jun 8, 2017

pmiddleton commented Jun 8, 2017

pmiddleton commented Jun 9, 2017

anpete commented Jun 9, 2017 • edited Loading

ajcvickers commented Jun 9, 2017

pmiddleton commented Jun 9, 2017

AndriySvyryd commented Jun 8, 2017 •

edited

Loading

anpete commented Jun 9, 2017 •

edited

Loading