Skip to content

Avoid unnecessary casting for Linq provider #2491

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

Merged
merged 21 commits into from
Sep 6, 2020
Merged

Avoid unnecessary casting for Linq provider #2491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
591f30e
Avoid unnecessary casting for Linq provider
maca88 Aug 17, 2020
05dd4d6
Fix tests
maca88 Aug 18, 2020
085a170
Avoid additional casting and add more test cases
maca88 Aug 21, 2020
492d1bd
Fix build
maca88 Aug 21, 2020
39cf3f5
Try fix firebird parameter regex
maca88 Aug 22, 2020
1aeaf37
Revert "Try fix firebird parameter regex"
maca88 Aug 22, 2020
c50939f
Skip failing tests for Firebird
maca88 Aug 22, 2020
010cf85
Fix odbc tests
maca88 Aug 22, 2020
c918327
Code review changes
maca88 Aug 25, 2020
a4d4df0
Code review changes
maca88 Aug 25, 2020
22c17ff
Update src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs
maca88 Aug 25, 2020
0858dce
Remove unused variable
hazzik Aug 27, 2020
6d76720
Extract GetParameterType and GetCandidateTypes methods
hazzik Aug 27, 2020
3bdb598
Reduce complexity
hazzik Aug 27, 2020
b4888b7
Further reduce complexity
hazzik Aug 27, 2020
e31a9a5
Move UnwrapUnary to ParameterTypeLocator as it is too specific
hazzik Aug 27, 2020
d10a057
Remove list creation
hazzik Aug 27, 2020
371b8df
It seems comment belongs here
hazzik Aug 27, 2020
9d0bbb5
Avoid HashSet creation for candidate type calculation
bahusoid Aug 27, 2020
3c10311
Move number type checks to TypeExtensions and extract additional meth…
hazzik Aug 27, 2020
503aec2
Merge to single GetCandidateType
bahusoid Aug 28, 2020
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
5 changes: 5 additions & 0 deletions src/NHibernate.DomainModel/Northwind/Entities/Northwind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public IQueryable<User> Users
get { return _session.Query<User>(); }
}

public IQueryable<NumericEntity> NumericEntities
{
get { return _session.Query<NumericEntity>(); }
}

public IQueryable<DynamicUser> DynamicUsers
{
get { return _session.Query<DynamicUser>(); }
Expand Down
29 changes: 29 additions & 0 deletions src/NHibernate.DomainModel/Northwind/Entities/NumericEntity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
namespace NHibernate.DomainModel.Northwind.Entities
{
public class NumericEntity
{
public virtual short Short { get; set; }

public virtual short? NullableShort { get; set; }

public virtual int Integer { get; set; }

public virtual int? NullableInteger { get; set; }

public virtual long Long { get; set; }

public virtual long? NullableLong { get; set; }

public virtual decimal Decimal { get; set; }

public virtual decimal? NullableDecimal { get; set; }

public virtual float Single { get; set; }

public virtual float? NullableSingle { get; set; }

public virtual double Double { get; set; }

public virtual double? NullableDouble { get; set; }
}
}
20 changes: 20 additions & 0 deletions src/NHibernate.DomainModel/Northwind/Mappings/NumericEntity.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?xml version="1.0" encoding="utf-8" ?>
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" namespace="NHibernate.DomainModel.Northwind.Entities" assembly="NHibernate.DomainModel">
<class name="NumericEntity">
<id name="Short">
<generator class="assigned" />
</id>

<property name="NullableShort" />
<property name="Integer" not-null="true" column="`Integer`" />
<property name="NullableInteger" />
<property name="Long" not-null="true" column="`Long`" />
<property name="NullableLong" />
<property name="Decimal" not-null="true" column="`Decimal`" />
<property name="NullableDecimal" />
<property name="Single" not-null="true" column="`Single`" />
<property name="NullableSingle" />
<property name="Double" not-null="true" column="`Double`" />
<property name="NullableDouble" />
</class>
</hibernate-mapping>
65 changes: 65 additions & 0 deletions src/NHibernate.Test/Async/Linq/NullComparisonTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Text;
using NHibernate.Dialect;
using NHibernate.Linq;
using NHibernate.DomainModel.Northwind.Entities;
using NUnit.Framework;
Expand Down Expand Up @@ -472,6 +473,33 @@ public async Task NullEqualityAsync()

await (ExpectAsync(session.Query<User>().Where(o => o.CreatedBy.ModifiedBy.Id == 5), Does.Not.Contain("is null").IgnoreCase));
await (ExpectAsync(session.Query<User>().Where(o => 5 == o.CreatedBy.ModifiedBy.Id), Does.Not.Contain("is null").IgnoreCase));

if (Sfi.Dialect is FirebirdDialect)
{
return;
}

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort == o.NullableShort), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short == o.Short), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort == o.Short), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short == o.NullableShort), WithoutIsNullAndWithoutCast()));

short value = 3;
await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort == value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value == o.NullableShort), WithoutIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort.Value == value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value == o.NullableShort.Value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short == value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value == o.Short), WithoutIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort == 3L), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3L == o.NullableShort), WithoutIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort.Value == 3L), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3L == o.NullableShort.Value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short == 3L), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3L == o.Short), WithoutIsNullAndWithoutCast()));
}

[Test]
Expand Down Expand Up @@ -560,6 +588,43 @@ public async Task NullInequalityAsync()

await (ExpectAsync(session.Query<User>().Where(o => o.CreatedBy.ModifiedBy.Id != 5), Does.Contain("is null").IgnoreCase));
await (ExpectAsync(session.Query<User>().Where(o => 5 != o.CreatedBy.ModifiedBy.Id), Does.Contain("is null").IgnoreCase));

if (Sfi.Dialect is FirebirdDialect)
{
return;
}

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort != o.NullableShort), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short != o.Short), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort != o.Short), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short != o.NullableShort), WithIsNullAndWithoutCast()));

short value = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary cast is exists short/long with == / <= / >= operators could you add additional test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When comparing two different types short == long, a cast will always be added when the mapped sql type of short and long are different. I am thinking of restoring the previous behavior of Equals in case a user wants to compare two different types without an explicit cast as this is not possible anymore in 5.3.

Copy link
Contributor

@gokhanabatay gokhanabatay Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the operators and methods should work exactly the same, otherwise the software developer might make an error about which one to use and under what condition. If not careful, sql clauses are created which can cause performance problems.

The strange thing is that it casts unnecessarily in short to short or long to long comparisons.

Equals method with == operator
CompareTo method result == 0 with == operator
CompareTo method result < 0 with <= operator
CompareTo method result > 0 with >= operator

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort != value), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value != o.NullableShort), WithIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort.Value != value), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value != o.NullableShort.Value), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short != value), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => value != o.Short), WithoutIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort != 3L), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3 != o.NullableShort), WithIsNullAndWithoutCast()));

await (ExpectAsync(db.NumericEntities.Where(o => o.NullableShort.Value != 3L), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3L != o.NullableShort.Value), WithIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => o.Short != 3L), WithoutIsNullAndWithoutCast()));
await (ExpectAsync(db.NumericEntities.Where(o => 3L != o.Short), WithoutIsNullAndWithoutCast()));
}

private IResolveConstraint WithIsNullAndWithoutCast()
{
return Does.Contain("is null").IgnoreCase.And.Not.Contain("cast").IgnoreCase;
}

private IResolveConstraint WithoutIsNullAndWithoutCast()
{
return Does.Not.Contain("is null").IgnoreCase.And.Not.Contain("cast").IgnoreCase;
}

[Test]
Expand Down
Loading