Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WIP Concat should return supplied string type #2966

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

WIP Concat should return supplied string type #2966

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f5f2613
Concat should return supplied string type
bahusoid Dec 28, 2021
efafac1
Fix ODBC
bahusoid Dec 28, 2021
377280a
Unskip sap tests for concat
hazzik Jan 1, 2022
79461cf
Trigger SAP build
hazzik Jan 1, 2022
b2fc289
Fix concat definition for SybaseSQLAnywhere10Dialect
hazzik Jan 2, 2022
dd9f1cf
Same as in Dialect
hazzik Jan 2, 2022
535fcb2
Revert changes to trim function
hazzik Jan 2, 2022
68b66f2
Revert HavingUsingSqlFunctions_Concat
hazzik Jan 2, 2022
0056a64
Merge branch 'master' into concat
bahusoid Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/NHibernate.Test/Async/Criteria/Lambda/FunctionsIntegrationFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,6 @@ public async Task FunctionsToLowerToUpperAsync()
[Test]
public async Task ConcatAsync()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

using (var s = OpenSession())
using (s.BeginTransaction())
{
Expand Down
9 changes: 1 addition & 8 deletions src/NHibernate.Test/Async/Criteria/ProjectionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ protected override void OnTearDown()
[Test]
public async Task UsingSqlFunctions_ConcatAsync()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

using (ISession session = Sfi.OpenSession())
{
string result = await (session.CreateCriteria(typeof(Student))
Expand All @@ -89,12 +86,8 @@ public async Task UsingSqlFunctions_ConcatAsync()
[Test]
public async Task UsingSqlFunctions_Concat_WithCastAsync()
{
if(Dialect is Oracle8iDialect)
{
if (Dialect is Oracle8iDialect)
Assert.Ignore("Not supported by the active dialect:{0}.", Dialect);
}
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

using (ISession session = Sfi.OpenSession())
{
Expand Down
53 changes: 53 additions & 0 deletions src/NHibernate.Test/Async/Hql/HQLFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,59 @@ public async Task ConcatAsync()
}
}

[Test]
public async Task ConcatAnsiFirstParamAsync()
{
if (!TestDialect.SupportsAnsiString)
Assert.Ignore("AnsiString type is not supported by dialect.");

var nickName = "abcdef";
using (ISession s = OpenSession())
using (var t = s.BeginTransaction())
{
var a1 = new Human { NickName = nickName, Birthdate = DateTime.Today };
await (s.SaveAsync(a1));
await (t.CommitAsync());
}

using (var logSpy = new SqlLogSpy())
using (ISession s = OpenSession())
{
var param = nickName + "gg";
var hql = "from Human a where concat(a.NickName, 'gg') = :param ";
Animal result = (Animal) await (s.CreateQuery(hql).SetParameter("param", param).UniqueResultAsync());
Assert.That(result, Is.Not.Null);
Assert.That(logSpy.GetWholeLog(), Does.Contain("Type: AnsiString"));
}
}

[KnownBug("Not fixed yet")]
[Test]
public async Task ConcatAnsiSecondParamAsync()
{
if (!TestDialect.SupportsAnsiString)
Assert.Ignore("AnsiString type is not supported by dialect.");

var nickName = "abcdef";
using (ISession s = OpenSession())
using (var t = s.BeginTransaction())
{
var a1 = new Human { NickName = nickName, Birthdate = DateTime.Today };
await (s.SaveAsync(a1));
await (t.CommitAsync());
}

using (var logSpy = new SqlLogSpy())
using (ISession s = OpenSession())
{
var param = "gg" + nickName;
var hql = "from Human a where concat('gg', a.NickName) = :param ";
Animal result = (Animal) await (s.CreateQuery(hql).SetParameter("param", param).UniqueResultAsync());
Assert.That(result, Is.Not.Null);
Assert.That(logSpy.GetWholeLog(), Does.Contain("Type: AnsiString"));
}
}

[Test]
public async Task HourMinuteSecondAsync()
{
Expand Down
3 changes: 0 additions & 3 deletions src/NHibernate.Test/Async/Linq/LinqQuerySamples.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1416,9 +1416,6 @@ public void ReplaceFunctionWithNullArgumentAsync()
[Test(Description = "GH-2860")]
public async Task StringFormatWithTrimAsync()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

var q =
from e in db.Employees
select new { Name = $"{e.FirstName} {e.LastName}".Trim(), Phone = e.Address.PhoneNumber };
Expand Down
3 changes: 0 additions & 3 deletions src/NHibernate.Test/Async/NHSpecificTest/NH1280/NH1280Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ protected override void OnTearDown()
[Test]
public async Task HavingUsingSqlFunctions_ConcatAsync()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

using (ISession s = OpenSession())
using (ITransaction tx = s.BeginTransaction())
{
Expand Down
3 changes: 0 additions & 3 deletions src/NHibernate.Test/Criteria/Lambda/FunctionsIntegrationFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,6 @@ public void FunctionsToLowerToUpper()
[Test]
public void Concat()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
hazzik marked this conversation as resolved.
Show resolved Hide resolved
Assert.Ignore("Current dialect does not support this test");

using (var s = OpenSession())
using (s.BeginTransaction())
{
Expand Down
9 changes: 1 addition & 8 deletions src/NHibernate.Test/Criteria/ProjectionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@ protected override void OnTearDown()
[Test]
public void UsingSqlFunctions_Concat()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
hazzik marked this conversation as resolved.
Show resolved Hide resolved
Assert.Ignore("Current dialect does not support this test");

using (ISession session = Sfi.OpenSession())
{
string result = session.CreateCriteria(typeof(Student))
Expand All @@ -78,12 +75,8 @@ public void UsingSqlFunctions_Concat()
[Test]
public void UsingSqlFunctions_Concat_WithCast()
{
if(Dialect is Oracle8iDialect)
{
if (Dialect is Oracle8iDialect)
Assert.Ignore("Not supported by the active dialect:{0}.", Dialect);
}
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
hazzik marked this conversation as resolved.
Show resolved Hide resolved
Assert.Ignore("Current dialect does not support this test");

using (ISession session = Sfi.OpenSession())
{
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate.Test/Hql/Animal.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<property name="Initial" column="name_initial"/>
<property name="Last" column="name_last"/>
</component>
<property name="NickName"/>
<property name="NickName" type="AnsiString(50)" />
<property name="Birthdate" type="Date"/>
</joined-subclass>
</class>
Expand Down
53 changes: 53 additions & 0 deletions src/NHibernate.Test/Hql/HQLFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,59 @@ public void Concat()
}
}

[Test]
public void ConcatAnsiFirstParam()
{
if (!TestDialect.SupportsAnsiString)
Assert.Ignore("AnsiString type is not supported by dialect.");

var nickName = "abcdef";
using (ISession s = OpenSession())
using (var t = s.BeginTransaction())
{
var a1 = new Human { NickName = nickName, Birthdate = DateTime.Today };
s.Save(a1);
t.Commit();
}

using (var logSpy = new SqlLogSpy())
using (ISession s = OpenSession())
{
var param = nickName + "gg";
var hql = "from Human a where concat(a.NickName, 'gg') = :param ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the second argument needs to be an Unicode string, because it contains some Unicode characters?
I do not think NHibernate has a convention for distinguishing ANSI literal strings from Unicode literal strings, which forces us to consider all literal strings to be Unicode strings, to be on the safe side. (By the way, does NHibernate use N'someString' for literal strings for SQL Server? I am not sure of that, and it may be a whole other issue.)
For me an adequate test would have to use arguments we all know for sure to be AnsiString, by example by using an AnsiString parameter as the second argument.
See SQL Server documentation: if any argument is an Unicode string, whatever its position, then the return type will be an Unicode string.

I also think this test is not the most relevant we could devise for the #1166 troubles. The performance trouble due to type mismatch occurs mainly when statistics or an index can be used by the query, which implies we need a raw column on one side of the comparison, not a column on which a function is called. In #1166, the example yields an SQL like FROM Person WHERE name like (@p0 + '%'). If we want to ascertain we are on the path of fixing the performance trouble, we need a test yielding something similar. (The like can be replaced by an equal comparison, of course.)

Animal result = (Animal) s.CreateQuery(hql).SetParameter("param", param).UniqueResult();
Assert.That(result, Is.Not.Null);
Assert.That(logSpy.GetWholeLog(), Does.Contain("Type: AnsiString"));
}
}

[KnownBug("Not fixed yet")]
[Test]
public void ConcatAnsiSecondParam()
{
if (!TestDialect.SupportsAnsiString)
Assert.Ignore("AnsiString type is not supported by dialect.");

var nickName = "abcdef";
using (ISession s = OpenSession())
using (var t = s.BeginTransaction())
{
var a1 = new Human { NickName = nickName, Birthdate = DateTime.Today };
s.Save(a1);
t.Commit();
}

using (var logSpy = new SqlLogSpy())
using (ISession s = OpenSession())
{
var param = "gg" + nickName;
var hql = "from Human a where concat('gg', a.NickName) = :param ";
Animal result = (Animal) s.CreateQuery(hql).SetParameter("param", param).UniqueResult();
Assert.That(result, Is.Not.Null);
Assert.That(logSpy.GetWholeLog(), Does.Contain("Type: AnsiString"));
}
}

[Test]
public void HourMinuteSecond()
{
Expand Down
3 changes: 0 additions & 3 deletions src/NHibernate.Test/Linq/LinqQuerySamples.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2001,9 +2001,6 @@ public void ReplaceFunctionWithNullArgument()
[Test(Description = "GH-2860")]
public void StringFormatWithTrim()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

var q =
from e in db.Employees
select new { Name = $"{e.FirstName} {e.LastName}".Trim(), Phone = e.Address.PhoneNumber };
Expand Down
3 changes: 0 additions & 3 deletions src/NHibernate.Test/NHSpecificTest/NH1280/NH1280Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ protected override void OnTearDown()
[Test]
public void HavingUsingSqlFunctions_Concat()
{
if (TestDialect.HasBrokenTypeInferenceOnSelectedParameters)
Assert.Ignore("Current dialect does not support this test");

using (ISession s = OpenSession())
using (ITransaction tx = s.BeginTransaction())
{
Expand Down
3 changes: 3 additions & 0 deletions src/NHibernate.Test/TestDialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Data;
using NHibernate.Hql.Ast.ANTLR;
using NHibernate.Id;
using NHibernate.SqlTypes;
Expand Down Expand Up @@ -54,6 +55,8 @@ public bool NativeGeneratorSupportsBulkInsertion

public virtual bool SupportsNullCharactersInUtfStrings => true;

public virtual bool SupportsAnsiString => _dialect.GetTypeName(new SqlType(DbType.AnsiString)) != _dialect.GetTypeName(new SqlType(DbType.String));

/// <summary>
/// Some databases do not support SELECT FOR UPDATE
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/Dialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ protected Dialect()
RegisterFunction("cast", new CastFunction());
RegisterFunction("transparentcast", new TransparentCastFunction());
RegisterFunction("extract", new AnsiExtractFunction());
RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "(", " || ", ")"));
RegisterFunction("concat", new VarArgsSQLFunction("(", " || ", ")"));
Copy link
Member

@fredericDelaporte fredericDelaporte Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an invalid change. It is not the first parameter which matters, but all parameters. As pointed in my first comment, all parameters must be ANSI strings for the return type to be an ANSI string. By not specifying the return type, we let NHibernate infer the returned type from the first parameter, which is then incorrect if the first is an ANSI string but not all others.

It is also worst to incorrectly type something as an ANSI string than incorrectly type something as an Unicode string: the former may cause garbled text and performance issues, while the later may only cause performance issues. (Well, since with SQL Server 2019, varchar/char can be UTF-8 encoded, the former in such case could not have the garbled text issue. But that remains a corner case for now I think.)

So I think this change is a regression.
We would need instead a specialized concat function which check the type of all its arguments, and would yield an ANSI string only if all of them are ANSI strings, otherwise it would yield an Unicode string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of simple comparison p.AnsiColumn = @param if @param type is not explicitly specified it will be treated as ANSI. Even though @param might contain some UTF-8 symbols...

To me this case is not much of a difference. It's just an assumption that's used for type detection for not specified explicitly parameter types. So I don't think any garbled text is possible here (only wrong comparisons results, the same as with simple comparison)

But OK let's be on a safe side...


// the syntax of current_timestamp is extracted from H3.2 tests
// - test\hql\ASTParserLoadingTest.java
Expand Down
1 change: 0 additions & 1 deletion src/NHibernate/Dialect/HanaDialectBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,6 @@ protected virtual void RegisterHANAFunctions()
RegisterFunction("ceil", new StandardSQLFunction("ceil"));
RegisterFunction("char", new StandardSQLFunction("char", NHibernateUtil.AnsiChar));
RegisterFunction("coalesce", new TypeConvertingVarArgsSQLFunction("coalesce(", ",", ")"));
RegisterFunction("concat", new VarArgsSQLFunction("(", " || ", ")"));
RegisterFunction("concat_naz", new StandardSQLFunction("concat_naz", NHibernateUtil.String));
RegisterFunction("convert_currency", new VarArgsSQLFunction("convert_currency(", ",", ")"));
RegisterFunction("convert_unit", new VarArgsSQLFunction("convert_unit(", ",", ")"));
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/MsSql2000Dialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ protected virtual void RegisterFunctions()
RegisterFunction("month", new SQLFunctionTemplate(NHibernateUtil.Int32, "datepart(month, ?1)"));
RegisterFunction("year", new SQLFunctionTemplate(NHibernateUtil.Int32, "datepart(year, ?1)"));
RegisterFunction("date", new SQLFunctionTemplate(NHibernateUtil.Date, "dateadd(dd, 0, datediff(dd, 0, ?1))"));
RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "(", "+", ")"));
RegisterFunction("concat", new VarArgsSQLFunction("(", "+", ")"));
RegisterFunction("digits", new StandardSQLFunction("digits", NHibernateUtil.String));
RegisterFunction("ascii", new StandardSQLFunction("ascii", NHibernateUtil.Int32));
RegisterFunction("chr", new StandardSQLFunction("char", NHibernateUtil.Character));
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/MsSqlCeDialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ protected virtual void RegisterFunctions()
RegisterFunction("trim", new AnsiTrimEmulationFunction());
RegisterFunction("iif", new IifSQLFunction());

RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "(", "+", ")"));
RegisterFunction("concat", new VarArgsSQLFunction("(", "+", ")"));
// Modulo is not supported on real, float, money, and numeric data types
RegisterFunction("mod", new ModulusFunctionTemplate(false));

Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/MySQLDialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ protected virtual void RegisterFunctions()
RegisterFunction("radians", new StandardSQLFunction("radians", NHibernateUtil.Double));
RegisterFunction("exp", new StandardSQLFunction("exp", NHibernateUtil.Double));

RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "concat(", ",", ")"));
RegisterFunction("concat", new VarArgsSQLFunction("concat(", ",", ")"));
RegisterFunction("replace", new StandardSafeSQLFunction("replace", NHibernateUtil.String, 3));
RegisterFunction("ltrim", new StandardSQLFunction("ltrim"));
RegisterFunction("rtrim", new StandardSQLFunction("ltrim"));
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/OracleLiteDialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public OracleLiteDialect()
RegisterFunction("user", new NoArgSQLFunction("user", NHibernateUtil.String, false));

// Multi-param string dialect functions...
RegisterFunction("concat", new StandardSQLFunction("concat", NHibernateUtil.String));
RegisterFunction("concat", new StandardSQLFunction("concat"));
RegisterFunction("instr", new StandardSQLFunction("instr", NHibernateUtil.String));
RegisterFunction("instrb", new StandardSQLFunction("instrb", NHibernateUtil.String));
RegisterFunction("lpad", new StandardSQLFunction("lpad", NHibernateUtil.String));
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Dialect/SybaseASE15Dialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public SybaseASE15Dialect()
RegisterFunction("bit_length", new SQLFunctionTemplate(NHibernateUtil.Int32, "datalength(?1) * 8"));
RegisterFunction("ceiling", new StandardSQLFunction("ceiling"));
RegisterFunction("char", new StandardSQLFunction("char", NHibernateUtil.String));
RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "(","+",")"));
RegisterFunction("concat", new VarArgsSQLFunction("(", "+", ")"));
RegisterFunction("cos", new StandardSQLFunction("cos", NHibernateUtil.Double));
RegisterFunction("cot", new StandardSQLFunction("cot", NHibernateUtil.Double));
RegisterFunction("current_date", new NoArgSQLFunction("current_date", NHibernateUtil.LocalDate));
Expand Down
1 change: 0 additions & 1 deletion src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ protected virtual void RegisterStringFunctions()
RegisterFunction("char_length", new StandardSQLFunction("char_length", NHibernateUtil.Int32));
RegisterFunction("compare", new VarArgsSQLFunction(NHibernateUtil.Int32, "compare(", ",", ")"));
RegisterFunction("compress", new VarArgsSQLFunction(NHibernateUtil.BinaryBlob, "compress(", ",", ")"));
RegisterFunction("concat", new VarArgsSQLFunction(NHibernateUtil.String, "(", "+", ")"));
Copy link
Member

@hazzik hazzik Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ behaves differently than || and requires explicit conversion to string first. It should be ||, but then definition becomes the same as in base dialect. So I just removed the overload.

RegisterFunction("csconvert", new VarArgsSQLFunction(NHibernateUtil.StringClob, "csconvert(", ",", ")"));
RegisterFunction("decompress", new VarArgsSQLFunction(NHibernateUtil.BinaryBlob, "decompress(", ",", ")"));
RegisterFunction("decrypt", new VarArgsSQLFunction(NHibernateUtil.BinaryBlob, "decrypt(", ",", ")"));
Expand Down