-
Notifications
You must be signed in to change notification settings - Fork 934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP - Improve select clause transformation for Linq provider #2079
base: master
Are you sure you want to change the base?
Conversation
Rebased to master and extended the purpose of this PR (description updated). |
c401cd9
to
5218dc7
Compare
Rebased and all tests should be now fixed. |
@@ -417,7 +417,7 @@ private void OverrideStandardHQLFunctions() | |||
RegisterFunction("nullif", new StandardSafeSQLFunction("nullif", 2)); | |||
RegisterFunction("lower", new StandardSafeSQLFunction("lower", NHibernateUtil.String, 1)); | |||
RegisterFunction("upper", new StandardSafeSQLFunction("upper", NHibernateUtil.String, 1)); | |||
RegisterFunction("mod", new StandardSafeSQLFunction("mod", NHibernateUtil.Double, 2)); | |||
RegisterFunction("mod", new StandardSafeSQLFunction("mod", NHibernateUtil.Int32, 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally depending on the dialect it should be either Int32 (FB <3.0) or Int64 (FB >3.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the documentation the result varies depending on the arguments. Also the arguments are rounded before appling the mod operator, which is not consistent with .NET and other databases. In the current master, the mod operator is never executed on the DB, which allows to consistently calculate the mod operation on the client side, but it prevents us from using it in a subquery. In this PR the mod operator is currently always executed on the database, which will not work correctly when using decimal values as arguments as all dialects have Int32
set as the return type. In order to fix that I will make custom ISQLFunction
classes for each dialect, which will have the information whether the mod operator can be translated for the given arguments or not. For example, in firebird when one of the arguments is decimal the mod operator will not be translated to sql in order to keep the consistency. As mod is probably not the only function which its result type varies depending on the arguments I am setting again to WIP, until the mod and other similar functions are fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried supplying null
as the type? In such case, NHibernate infers the type from the first argument of the function. But maybe you need to do that according to the second argument, or considering both, in which case some changes a bit deeper will be required. (See FindFunctionReturnType
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I tried, but in case of mod
function the returning type depends on both parameters. I am intending to add an overload for FindFunctionReturnType
which will take all function arguments and also extend ISQLFunction
with a transitional interface that will do the same for ReturnType
mehod. Unfurtunately, the new ReturnType
method will not be enough, because the same method is called also for a hql query that may not have the information about the type. For example:
var q = session.CreateQuery("select mod(a.Id, :p1) from Animal a")
.SetParameter("p1", 2.1m);
In the above query, the hql string is analyzed (FindFunctionReturnType
gets called) when the CreateQuery
method is called, which means that we do not have the type of the paramater. For this reason the transitional ISQLFunction
interface will need to have an additional property DefaultReturnType
which will be used when arguments types cannot be determined. So for the given example, all values will be converted to int
, because the default return type for the mod
function is set to Int32
, which is also how it currently works. For a linq query the parameter will be wrapped with a transparentcast
in order to preserve the type when building the hql tree, so that FindFunctionReturnType
will be able to determine all parameter types.
case TypeCode.Double: | ||
return true; | ||
default: | ||
return new[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to replace with just if-elif-else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified it by using the ||
operator.
Due to multiple issues that I've encountered along the way to address the parameter type issue for methods that have more than one parameters, I expanded what this PR does. For better understanding of the changes that were made in the last commit, I will try to explain what and why was changed. As previously mentioned,
Entities that implement
When some implementors have the The next thing that was changed is how the hql tree is created for Linq queries and when parameter types are detected. Prior this PR, the parameter types were all detected when translating the hql tree except for parameters that are using
in the above example the hql nominates would be all three
the above query will execute the expression inside
the above query will execute the As this PR now depends on #2036, I will leave it to WIP. |
Rebased on top of #2036 and fixed Odbc tests by moving the logic of setting the parameter size from |
Co-authored-by: Alexander Zaytsev <[email protected]>
_useBinaryFloatingPointTypes = PropertiesHelper.GetBoolean( | ||
Environment.OracleUseBinaryFloatingPointTypes, | ||
settings, | ||
false); | ||
|
||
base.Configure(settings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I messed up this in #2349.
@@ -432,7 +432,7 @@ private void OverrideStandardHQLFunctions() | |||
{ | |||
RegisterFunction("current_timestamp", new CurrentTimeStamp()); | |||
RegisterFunction("current_date", new NoArgSQLFunction("current_date", NHibernateUtil.LocalDate, false)); | |||
RegisterFunction("length", new StandardSafeSQLFunction("char_length", NHibernateUtil.Int64, 1)); | |||
RegisterFunction("length", new StandardSafeSQLFunction("char_length", NHibernateUtil.Int32, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned with other dialects.
@@ -31,6 +31,11 @@ public IBodyClause Clone(CloneContext cloneContext) | |||
return new NhOuterJoinClause(JoinClause.Clone(cloneContext)); | |||
} | |||
|
|||
public override string ToString() | |||
{ | |||
return $"outer {JoinClause}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add it in #2328, which is needed for creating unique cache keys.
Squashed and rebased. |
Example:
Select(o => o.ServerSideEval.ClientSideEval)
-> NRE will not be thrown whenServerSideEval
isnull
ListInit
andInvoke
expression typesFixes #2234
Fixes #2334