Skip to content

NH-3848 - Tests and fix for HQL #463

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
Merged
merged 19 commits into from
Feb 6, 2019
Merged

NH-3848 - Tests and fix for HQL #463

merged 19 commits into from
Feb 6, 2019

Conversation

zalewskil
Copy link
Contributor

@zalewskil zalewskil commented Feb 24, 2016 •
edited by fredericDelaporte
Loading

This pull should be treated as a hint for fix or tests rather than actual fix (but maybe I'm wrong).
It can harm performance (traversing Where clause tree or using String.StartsWith).
Actually we aren't using HQL in our application so this fix had to be done quick.

NH-3848.

Fixes #1341

@hazzik hazzik assigned hazzik and unassigned hazzik Jul 25, 2017
@nhibernate-bot nhibernate-bot mentioned this pull request Oct 13, 2017
Closed
@hazzik hazzik changed the title Nh 3848 - Tests and fix for HQL NH-3848 - Tests and fix for HQL May 25, 2018
@hazzik
Copy link
Member

hazzik commented Nov 23, 2018

Rebased to resolve conflicts

@hazzik

This comment has been minimized.

Sign in to view
hazzik
hazzik reviewed Nov 23, 2018
View reviewed changes
src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
}
}

public bool CanAddFetchedCollectionToCache
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs to be moved to a loader, however, I'm not entirely sure.

Copy link
Member

@fredericDelaporte fredericDelaporte Nov 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't it be CanAddFetchedCollectionsToCache? (Collection is switched to plural.)

If I understand well this PR, contrary to #460, it would disable collection caching for the whole query, not just for the collection having restrictions.

This probably needs to be moved to a loader, however, I'm not entirely sure.

I am not sure either. It may require to expose some internal state of the translator for the loader to analyse it, and then, better keep it in the translator.

@hazzik hazzik changed the title NH-3848 - Tests and fix for HQL WIP - NH-3848 - Tests and fix for HQL Nov 23, 2018
fredericDelaporte
fredericDelaporte reviewed Nov 24, 2018
View reviewed changes
src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
}
}

public bool CanAddFetchedCollectionToCache
Copy link
Member

@fredericDelaporte fredericDelaporte Nov 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't it be CanAddFetchedCollectionsToCache? (Collection is switched to plural.)

If I understand well this PR, contrary to #460, it would disable collection caching for the whole query, not just for the collection having restrictions.

This probably needs to be moved to a loader, however, I'm not entirely sure.

I am not sure either. It may require to expose some internal state of the translator for the loader to analyse it, and then, better keep it in the translator.

bahusoid
bahusoid reviewed Nov 24, 2018
View reviewed changes
fredericDelaporte and others added 3 commits November 24, 2018 17:07
@fredericDelaporte
Guarantee session closing and use common test base
11ed4ec 
Also add transactions since using second level cache without
transactions is a bad practice.
@fredericDelaporte
Do minor cleanup
de8caf0
@bahusoid
Attempt to optimize restriction search logic
2f66408
fredericDelaporte
fredericDelaporte reviewed Nov 25, 2018
View reviewed changes
Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

@bahusoid change is a good improvement in my opinion.

bahusoid
bahusoid reviewed Dec 18, 2018
View reviewed changes
hazzik
hazzik reviewed Dec 27, 2018
View reviewed changes
bahusoid
bahusoid previously approved these changes Dec 29, 2018
View reviewed changes
Copy link
Member

@bahusoid bahusoid left a comment

Choose a reason for hiding this comment

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

Why WIP?

@hazzik hazzik changed the title WIP - NH-3848 - Tests and fix for HQL NH-3848 - Tests and fix for HQL Jan 14, 2019
@fredericDelaporte fredericDelaporte added this to the 5.3 milestone Jan 25, 2019
@bahusoid
Copy link
Member

bahusoid commented Feb 4, 2019

But it seems to fully fix NH-3848 filter case (when collection is filtered by session.EnableFilter) is also needs to be handled (both in criteria and hql)

@fredericDelaporte
Copy link
Member

But it seems to fully fix NH-3848 filter case (when collection is filtered by session.EnableFilter) is also needs to be handled (both in criteria and hql).

Let's call it another bug, if confirmed.

@fredericDelaporte fredericDelaporte merged commit ac55892 into nhibernate:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazzik hazzik hazzik left review comments

@fredericDelaporte fredericDelaporte fredericDelaporte left review comments

@bahusoid bahusoid bahusoid approved these changes

Assignees
No one assigned
Labels
r: Fixed t: Fix
Projects
None yet
Milestone
5.3
5 participants
@zalewskil @hazzik @bahusoid @fredericDelaporte @itmagination