UDFs and SqlFunction quoting and schema logic

[roji]

While implementing the tests for UDFs (yeah I'm a bit late to the party...), I came across some odd quoting/schema logic for SQL functions that causes a problem in PostgreSQL - this was implemented in #9558. I understand the problems it was meant to solve, but that doesn't really work from a cross-database perspective.

So in https://github.com/aspnet/EntityFrameworkCore/blob/ea9b3d84f9028a8a5eb35ae02babd50836947163/src/EFCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs#L1548, it seems that GenerateSqlFunctionName() passes the function name through DelimitIdentifier() only if a schema has been defined on it. This in itself is a bit strange - why should function name quoting depend on whether a schema has been defined for that function? Note that in PostgreSQL function names behave exactly like other identifiers (e.g. table names) and need to be quoted if they contain upper-case letters.

The way things are implemented in SQL Server, it turns out that functions implicitly have a schema - SqlServerDbFunctionConvention sets the DefaultSchema of all functions to be dbo (#9303). This makes generated SQL contain [dbo].[FunctionName](...) instead of a simpler/leaner [FunctionName]. This seems to go against EF Core's principle of generating clean, concise SQL that resembles what we'd write by hand; after all why specify dbo everywhere for functions but not for tables? Is this some sort of SQL Server-specific requirement?

Setting up the same convention in Npgsql (with default schema public) makes the 1st problem go away for some cases, since functions now have a schema by default. But it doesn't handle the case where the user explicitly sets an empty schema, like for IsDate in the tests - since there's no schema there's no quoting, and PostgreSQL fails to find isdate after converting the name to lowercase.

For now, my approach is to override DefaultQuerySqlGenerator.GenerateSqlFunctionName() to make it always quote. This creates an issue for some built-in function which EF Core generates upper-case (e.g. SUM), so I have a hardcoded list of built-in functions that aren't to be quoted (this would be obviated if EF Core starts generating lower-case sum() instead of SUM()). I'm also modifying all my expression translators to generate function names in lower-case (e.g. lower() instead of LOWER()).

I realize this is a bit late to be raising this (we should have thought about PostgreSQL when doing UDFs and #9558), just letting you know.

/cc @pmiddleton

[pmiddleton]

Welcome to the wonderful world of quirky Sql Server syntax.

Sql Server differentiates calls to built in functions vs user defined functions based on the presence of the schema name. If a function call contains a schema name then Sql Server tries to resolve it as a UDF. Without a schema name and it tries to resolve it as a built in function. This is why SqlServerDbFunctionConvention sets the default schema.

On top of that you also are not allowed to escape built in function calls in Sql Server. This is illegal [IsDate](...) it has to be IsDate(...)

The current solution in place is to look at the presence of the schema to know whether or not to escape the function. If you read my comment on #9558 I was concerned this solution might not work for all providers.

@divega - The uppercase/lowercase issue (SUM vs sum) for PostgreSQL is another use case for why we need a metadata store for built in functions which can be modified at run time.

I think we need a way to flag a DbFunction/SqlFunctionExpression to know if it is a built in function vs UDF so providers can make appropriate decisions on rendering them.

[roji]

Oh, thanks for that explanation... That explains some of the weirdness I saw this weekend :)

Yeah, some sort of flag on the function call would be good here - I was just about to add a NeedsQuoting flag on my own provider-specific function expression.

On the PostgreSQL sides things are a bit simpler but still a bit quirky. Built-in functions aren't special as far as I'm aware - they're just functions in the public (default) schema. It's totally fine to quote any function in order to preserve case (otherwise PostgreSQL folds to lowercase), or in case the function name has special characters.

Even in the PostgreSQL case it may make sense to add a "built-in" flag, to avoid quoting under any circumstances. This way, a method translator can specify uppercase SUM and have that go as-is into the SQL, to be folded to lowercase by PG (if it gets quoted PG will error because the actual function is lowercase sum). This can be useful to make the SQL prettier/more readable.

[ajcvickers]

@roji Are you considering submitting a PR for this?

[roji]

I guess I can - the idea would be to add an IsUserDefined property to SqlFunctionExpression (only true in instances returned by DbFunction), and look at that when generating SQL for calling it? For SQL Server (and PostgreSQL) only user-defined functions would be quoted... Sounds reasonable to everyone?

[pmiddleton]

There is still an issue with Sql Server. You are allowed to map a built in function using a UDF. We really need to carry the IsUserDefined through to IDbFunction and the appropriate fluent configuration methods.

Without doing this we have to continue to rely on the presence of a supplied schema to make the decision which is not ideal.

[roji]

@pmiddleton exposing this built-in/UDF distinction in the fluent APIs isn't ideal because it seems to be an SQL Server-specific concern (at least in PostgreSQL it's pretty meaningless)...

Maybe it's just better to leave things as they are? I can always handle it internally in the Npgsql provider (i.e. always use lower-case names etc.)? Or do you think we can do better in the general case?

[pmiddleton]

Really neither solution is ideal for Sql Server. On one hand you have to know of the behavior of how schema is used (udf vs built in) on the other you have a fluent setting that doesn't make a ton of sense.

Ideally @ajcvickers moves the EF fort and lays siege to the Sql Server team until they change TSQL :)

I really don't see a good generalized solution to this right now given how different each system is, combined with the ability to use UDFs to map built in functions.

[ajcvickers]

@pmiddleton That's @divega's job. ;-)

[roji]

Yeah, it seems like there's nowhere to go with this really, I'd definitely prefer for EF Core's public API and general internals to stay clean and for any hacks to be completely SQL Server-specific, which is more or less the current situation. So I'm going to close this for now, unless anyone has any better idea.

[divega]

FWIW, in EF6, function metadata contains a flag to indicate whether it is built-in, alongside the ones that indicate whether they are niladic, aggregate, conposable, etc. So it wouldn’t be the first time this problem has been approached that way.

[roji]

Reopening to rethink.

If I try hard I can see the utility of a fluent-API-exposed IsBuiltIneven for PostgreSQL, meaning simply that the function is never to be quoted, like in SQL Server. Unlike SQL Server this isn't really necessary, but would allow users to specify function names in any case without fear of them being quoted, which is nice for readability. This could be useful for functions with names such as ST_AsGeometry (if you add quotes around them it has to be "st_asgeometry"). So there does seem to be at least some cross-database justification for this.

If that makes sense to people I can submit a PR for that.

I can also submit a PR for niladic, which also makes sense for PostgreSQL (e.g. current_timestamp which can only be called without parentheses).

[pmiddleton]

What name do we want to go with? I'm not sure if IsBuiltIn is descriptive enough. IsServer is another option but I think that is even more confusing. Have I mentioned that I hate naming things.

Are these fluent only options or should DbFunctionAttribute support them. I think they are advanced options so fluent only is ok.

Thoughts on this @divega

[roji]

Another naming option is to flip the logic and have IsUserDefined - maybe that's more descriptive (not sure).

[roji]

@ajcvickers any specific reason this needs to wait for 3.0.0 (aside from workload)? If not I can work on a PR for 2.2.

[roji]

(I'm assuming this can be done without breaking backwards compatibility, which I admit doesn't seem very clear)

[ajcvickers]

@roji I think we would consider a PR before 3.0. I believe it is in 3.0 because it involves query, for which we are focusing initially on #12795 since depending on the outcome of that issue things done in query may or may not need to be redone if we do them before. @smitpatel can confirm if there is any other reason for pushing this to 3.0.

[smitpatel]

Design meeting notes:
SqlServer

• BuiltIn functions cannot be quoted
• Non-builtIn functions must be quoted
• Functions are case insensitive
  SQLite
• Functions with name containing space etc has to quoted
• Anything else does not matter quoted or not
  Postgre
• Functions are case sensitive
• Needs to be quoted to preserve case or with special character
• Can remove quoting

Options:

• IsBuiltIn flag
  • SqlServer can look at the flag and determine.
  • On the top, providers can look at the string and can determine to quote or not.
• List of built in function in providers
  • Requires full list of functions. Over the time more may be added. Spatial kind package brings in more functions.
• FunctionName must be passed quoted. UDF functionName would be quoted automatically.
  • Inconsistent with other parts of stack.

Decision:
We will add IsBuiltIn flag on SqlFunctionExpression. It is required argument unless we see huge demand for default.

[pmiddleton]

@smitpatel - We need to carry the IsBuiltIn flag into the DbFunctions fluent api / attribute. That way we can properly deal with builtin functions that are mapped by the end user.

I can do that once we agree that is the way we want to go.

[smitpatel]

We added IsBuitIn flag which will determine if function would be quoted or not.