Query: SqlFunctionExpression API review

[smitpatel]

In new query pipeline, in order to facilitate creating different function, currently following variations are supported. (Also there niladic version)

Name(…)
dbo.Name(…)
Instance.Name(…)

Following are all the constructors. (You can ignore type, typeMapping, condition parameters, they are metadata)

public SqlFunctionExpression(
            string functionName,
            bool niladic,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(null, null, functionName, niladic, null, type, typeMapping, condition)
        {
        }

        public SqlFunctionExpression(
            string schema,
            string functionName,
            bool niladic,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(null, schema, functionName, niladic, null, type, typeMapping, condition)
        {
        }

        public SqlFunctionExpression(
            SqlExpression instance,
            string functionName,
            bool niladic,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(instance, null, functionName, niladic, null, type, typeMapping, condition)
        {
        }

        public SqlFunctionExpression(
            string functionName,
            IEnumerable<SqlExpression> arguments,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(null, null, functionName, false, arguments, type, typeMapping, condition)
        {
        }

        public SqlFunctionExpression(
            string schema,
            string functionName,
            IEnumerable<SqlExpression> arguments,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(null, schema, functionName, false, arguments, type, typeMapping, condition)
        {
        }

        public SqlFunctionExpression(
            SqlExpression instance,
            string functionName,
            IEnumerable<SqlExpression> arguments,
            Type type,
            RelationalTypeMapping typeMapping,
            bool condition)
            : this(instance, null, functionName, false, arguments, type, typeMapping, condition)
        {
        }

Is above is exhaustive? Do we need to support more variations? Is dbo.Name (niladic) or Instance.Name (niladic) is useful or possible or impossible?

cc: @bricelam due to SQLite & Spatial
cc: @roji due to Postgre
cc: @pmiddleton based on all the work with DbFunction + pivot + TVF.

[roji]

Check out especially #12757 for a fascinating (and depressing) discussion... Basically SQL Server seems to require a distinction between system and user functions - this affects both quoting and schema handling. FWIW PostgreSQL could also benefit from this distinction (we'd generate uppercase but unquoted names for system functions, so we can get SUM() instead of sum() in SQL). So it might be a good idea to add IsBuiltin.

Note also the EF6 function definition which also contains IsAggregate and IsComposable. If we really want to go all-out we can also think about PostgreSQL variadic functions (like params in C#), but that's definitely a rare edge-case we can support later.

Just for the added context, I'm not really aware of OOP-style functions in PostgreSQL (i.e. Instance.Name()).

[roji]

(let me know if you think you need any other info)

[bricelam]

Is dbo.Name (niladic) or Instance.Name (niladic) is useful or possible?

Instance.Name (niladic) was added for SQL Server spatial:

• Geometry.X
• Geometry.Y
• Geometry.Z
• Geometry.M
• Geometry.SRID
• Geography.Lat
• Geography.Long

[bricelam]

dbo.Name (niladic) feels like YAGNI

[smitpatel]

If we really want to go all-out we can also think about PostgreSQL variadic functions (like params in C#), but that's definitely a rare edge-case we can support later.

There is always possibility for any provider/plugin-writer/customer to extend our SQL tree and define a custom SqlFunctionExpression which supports such scenarios. There is no blockers. This is just what we provide "in-the-box".

As for variadic functions, it is just same function with different number of args. I believe at translation time we know exactly how many arguments are there (from LINQ expression tree). Further, we don't have checks on number of args so you can use same functionName and pass in whatever number of args you want to print out. So variadic functions should be supported already.

IsBuiltIn - I will read the "depressing" discussion and think about possible options. 😨
IsAggregate - would be useful for UDFs returning aggregate which can be used inside GroupBy query.
IsComposable - does anyone know what it was for?

[roji]

@divega can probably provide a bit more info on IsComposable.

[pmiddleton]

Yea Sql Server is a pain. Read #12757 to understand all the nuances.

What it boils down to is if we want to guarantee not to produce any potential bad sql we need to know if a custom DbFunction maps back to a UDF or built in function.

It's simple enough to ask for the data, but it just presents an ugly api option to the end user which they won't understand. I'm sure we can write some thoughtfully cryptic documentation which will confuse them even more :)

[smitpatel]

I had chat with @divega about IsComposable flag. It was because those functions also represented stored procedures. Given current support & design of stored proc in EF Core, we don't need that flag as of now. We can add it in future as needed.

[smitpatel]

Added IsBuiltIn flag on SqlFunctionExpression.
Only constructor which takes schema has ability to specify built-in or not. Rest would be assumed to be built-in. Niladic & Instance methods cannot be UDFs. When schema is optional, schema-less ctor can be used to create UDF but user can opt into specifying null schema in the other constructor.