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 1 commit
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
70 changes: 9 additions & 61 deletions src/NHibernate.Test/Async/Linq/ParameterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,7 @@ public async Task CompareIntegralParametersAndColumnsAsync()
3,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(3));
}));
}
Expand Down Expand Up @@ -211,15 +203,7 @@ public async Task CompareIntegralParametersWithFloatingPointColumnsAsync()
3,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(3));
}));
}
Expand Down Expand Up @@ -261,15 +245,7 @@ public async Task CompareFloatingPointParametersAndColumnsAsync()
totalParameters,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(totalParameters));
}));
}
Expand Down Expand Up @@ -392,17 +368,8 @@ public async Task CompareFloatingPointParameterWithDifferentFloatingPointColumns
var matches = pair.Value == "Double"
? Regex.Matches(sql, @"cast\([\w\d]+\..+\)")
: Regex.Matches(sql, @"cast\(((@|\?|:)p\d+|\?)\s+as.*\)");
if (Sfi.Dialect is FirebirdDialect)
{
// Additional casts are added by FirebirdClientDriver
Assert.That(matches.Count, Is.EqualTo(pair.Value == "Double" ? 1 : 2));
}
else
{
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(matches.Count, Is.EqualTo(sameType && !(Sfi.Dialect is SQLiteDialect) ? 0 : 1));
}

// SQLiteDialect uses sql cast for transparentcast method
Assert.That(matches.Count, Is.EqualTo(sameType && !(Sfi.Dialect is SQLiteDialect) ? 0 : 1));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(1));
}));
}
Expand Down Expand Up @@ -441,7 +408,7 @@ public async Task CompareIntegralParameterWithIntegralAndFloatingPointColumnsAsy
sql =>
{
var matches = Regex.Matches(sql, @"cast\(((@|\?|:)p\d+|\?)\s+as.*\)");
Assert.That(matches.Count, Is.EqualTo(Sfi.Dialect is FirebirdDialect ? 2 : 1));
Assert.That(matches.Count, Is.EqualTo(1));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(1));
}));
}
Expand Down Expand Up @@ -480,17 +447,7 @@ public async Task UsingValueTypeParameterOfDifferentTypeAsync()
await (AssertTotalParametersAsync(
query,
1,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}
}));
sql => Assert.That(sql, Does.Not.Contain("cast"))));
}

queriables = new List<IQueryable<NumericEntity>>
Expand Down Expand Up @@ -519,16 +476,7 @@ public async Task UsingValueTypeParameterOfDifferentTypeAsync()
sql => {
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(sql, !sameType || Sfi.Dialect is SQLiteDialect ? Does.Match("where\\s+cast") : (IResolveConstraint)Does.Not.Contain("cast"));
if (Sfi.Dialect is FirebirdDialect)
{
// Additional casts are added by FirebirdClientDriver
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(3));
}
else
{
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(!sameType || Sfi.Dialect is SQLiteDialect ? 1 : 0));
}
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(!sameType || Sfi.Dialect is SQLiteDialect ? 1 : 0));
}));
}
}
Expand All @@ -541,7 +489,7 @@ public async Task UsingValueTypeParameterTwiceOnNullablePropertyAsync()
db.NumericEntities.Where(o => o.NullableShort == value && o.NullableShort != value && o.Short == value),
1, sql => {

Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(Sfi.Dialect is FirebirdDialect ? 3 : 0));
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(0));
}));
}

Expand Down
70 changes: 9 additions & 61 deletions src/NHibernate.Test/Linq/ParameterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,7 @@ public void CompareIntegralParametersAndColumns()
3,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(3));
});
}
Expand Down Expand Up @@ -199,15 +191,7 @@ public void CompareIntegralParametersWithFloatingPointColumns()
3,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(3));
});
}
Expand Down Expand Up @@ -249,15 +233,7 @@ public void CompareFloatingPointParametersAndColumns()
totalParameters,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}

Assert.That(sql, Does.Not.Contain("cast"));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(totalParameters));
});
}
Expand Down Expand Up @@ -380,17 +356,8 @@ public void CompareFloatingPointParameterWithDifferentFloatingPointColumns()
var matches = pair.Value == "Double"
? Regex.Matches(sql, @"cast\([\w\d]+\..+\)")
: Regex.Matches(sql, @"cast\(((@|\?|:)p\d+|\?)\s+as.*\)");
if (Sfi.Dialect is FirebirdDialect)
{
// Additional casts are added by FirebirdClientDriver
Assert.That(matches.Count, Is.EqualTo(pair.Value == "Double" ? 1 : 2));
}
else
{
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(matches.Count, Is.EqualTo(sameType && !(Sfi.Dialect is SQLiteDialect) ? 0 : 1));
}

// SQLiteDialect uses sql cast for transparentcast method
Assert.That(matches.Count, Is.EqualTo(sameType && !(Sfi.Dialect is SQLiteDialect) ? 0 : 1));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(1));
});
}
Expand Down Expand Up @@ -429,7 +396,7 @@ public void CompareIntegralParameterWithIntegralAndFloatingPointColumns()
sql =>
{
var matches = Regex.Matches(sql, @"cast\(((@|\?|:)p\d+|\?)\s+as.*\)");
Assert.That(matches.Count, Is.EqualTo(Sfi.Dialect is FirebirdDialect ? 2 : 1));
Assert.That(matches.Count, Is.EqualTo(1));
Assert.That(GetTotalOccurrences(sql, $"Type: {pair.Value}"), Is.EqualTo(1));
});
}
Expand Down Expand Up @@ -468,17 +435,7 @@ public void UsingValueTypeParameterOfDifferentType()
AssertTotalParameters(
query,
1,
sql =>
{
if (Sfi.Dialect is FirebirdDialect)
{
Assert.That(sql, Does.Contain("cast"));
}
else
{
Assert.That(sql, Does.Not.Contain("cast"));
}
});
sql => Assert.That(sql, Does.Not.Contain("cast")));
}

queriables = new List<IQueryable<NumericEntity>>
Expand Down Expand Up @@ -507,16 +464,7 @@ public void UsingValueTypeParameterOfDifferentType()
sql => {
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(sql, !sameType || Sfi.Dialect is SQLiteDialect ? Does.Match("where\\s+cast") : (IResolveConstraint)Does.Not.Contain("cast"));
if (Sfi.Dialect is FirebirdDialect)
{
// Additional casts are added by FirebirdClientDriver
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(3));
}
else
{
// SQLiteDialect uses sql cast for transparentcast method
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(!sameType || Sfi.Dialect is SQLiteDialect ? 1 : 0));
}
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(!sameType || Sfi.Dialect is SQLiteDialect ? 1 : 0));
});
}
}
Expand All @@ -529,7 +477,7 @@ public void UsingValueTypeParameterTwiceOnNullableProperty()
db.NumericEntities.Where(o => o.NullableShort == value && o.NullableShort != value && o.Short == value),
1, sql => {

Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(Sfi.Dialect is FirebirdDialect ? 3 : 0));
Assert.That(GetTotalOccurrences(sql, "cast"), Is.EqualTo(0));
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/NHibernate/Driver/FirebirdClientDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public class FirebirdClientDriver : ReflectionBasedDriver
// Zero-width negative look-behind: the match must not be preceded by
@"(?<!" +
// a comparison,
@"[=<>]\s" +
@"[=<>]\s*" +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the possible regex fix for Firebird as there are other tests that would need to be modified, so tests are now skipped instead. Maybe a better fix would be to use transparentcast, similar what was done for SqLite.

// or a paging instruction,
@"|\bfirst\s+|\bskip\s+" +
// or a "between" condition,
Expand All @@ -36,7 +36,7 @@ public class FirebirdClientDriver : ReflectionBasedDriver
// Zero-width negative look-ahead: the match must not be followed by
@"(?!" +
// a comparison.
@"\s[=<>])";
@"\s*[=<>])";
private static readonly Regex _statementRegEx = new Regex(SELECT_CLAUSE_EXP, RegexOptions.IgnoreCase);
private static readonly Regex _castCandidateRegEx = new Regex(CAST_PARAMS_EXP, RegexOptions.IgnoreCase);
private readonly FirebirdDialect _fbDialect = new FirebirdDialect();
Expand Down