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

Enable Not node handling in HqlParser.NegateNode #3390

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Enable Not node handling in HqlParser.NegateNode #3390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
969244e
Test case
bahusoid Aug 1, 2023
08a6360
Fix
bahusoid Aug 1, 2023
550b3cb
Generate async files
github-actions[bot] Aug 1, 2023
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
16 changes: 16 additions & 0 deletions src/NHibernate.Test/Async/NHSpecificTest/GH3327/Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,21 @@ WHERE NOT (
)");
Assert.That((await (q.ListAsync()))[0], Is.EqualTo(0));
}

[Test]
public async Task NotNotExistsNegatedAsync()
{
using var log = new SqlLogSpy();
using var session = OpenSession();
var results = await (session.CreateQuery(
@"SELECT COUNT(ROOT.Id)
FROM Entity AS ROOT
WHERE NOT (
NOT EXISTS (FROM ChildEntity AS CHILD WHERE CHILD.Parent = ROOT)
AND NOT ROOT.Name = 'Parent'
)").ListAsync());
Assert.That(log.GetWholeLog(), Does.Not.Contains(" NOT ").IgnoreCase);
Assert.That(results.Count, Is.EqualTo(1));
}
}
}
16 changes: 16 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH3327/Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,21 @@ WHERE NOT (
)");
Assert.That(q.List()[0], Is.EqualTo(0));
}

[Test]
public void NotNotExistsNegated()
{
using var log = new SqlLogSpy();
using var session = OpenSession();
var results = session.CreateQuery(
@"SELECT COUNT(ROOT.Id)
FROM Entity AS ROOT
WHERE NOT (
NOT EXISTS (FROM ChildEntity AS CHILD WHERE CHILD.Parent = ROOT)
AND NOT ROOT.Name = 'Parent'
)").List();
Assert.That(log.GetWholeLog(), Does.Not.Contains(" NOT ").IgnoreCase);
Comment on lines +58 to +64
Copy link
Member

Choose a reason for hiding this comment

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

This test is about a more complex case than the comment in the parser: not (not a and not b) instead of not (not x). That is not a simple double not. Eliminating the not requires to change it to (a or b). Is it what is actually done?

Copy link
Member Author

@bahusoid bahusoid Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Eliminating the not requires to change it to (a or b). Is it what is actually done?

Yes. #3328 was actually about proper handling of and/or negation. And this test case just makes sure that we don't generate double not anymore.

Copy link
Member

Choose a reason for hiding this comment

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

I have now locally checked. It does also add required parenthesis if needed, when the condition is more complex (like not (not a and not b) and c).

Assert.That(results.Count, Is.EqualTo(1));
}
}
}
6 changes: 2 additions & 4 deletions src/NHibernate/Hql/Ast/ANTLR/HqlParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,10 +300,8 @@ public IASTNode NegateNode(IASTNode node)
node.Type = BETWEEN;
node.Text = "{not}" + node.Text;
return node; // (NOT (NOT_BETWEEN a b) ) => (BETWEEN a b)
/* This can never happen because this rule will always eliminate the child NOT.
case NOT:
return x.getFirstChild(); // (NOT (NOT x) ) => (x)
*/
case NOT:
return node.GetChild(0); // (NOT (NOT x) ) => (x)
default:
IASTNode not = (IASTNode) TreeAdaptor.Create(NOT, "not");
not.AddChild(node);
Expand Down
Loading