Combine Future queries for ICriteria and IQuery #1718

bahusoid · 2018-05-31T09:47:05Z

The intention is to implement batcher that can contain both ICriteria and IQuery based queries. And make Future implementation based on this batcher. See examples in tests

New batcher interface IQueryBatch (implementation QueryBatch ) allows only to Add queries wrapped in to IQueryBatchItem interface to batch and Execute batch.

See implementation for Criteria, IQuery and Linq. Most of functionality is shared in QueryBatchItemBase .

Caching is implemented per query (it's up to IQueryBatchItem implementation to cache or not to cache). So it's possible to cache queries with different settings (cache region etc), combine cached and non-cached queries. And batcher doesn't need to know all internal details about query cache (so it might be different cache for different IQueryBatchItem implementations).

Fixes #752

As "side effects" of this re-implementation of futures:

Supports stateless session
Fixes NH-2428 - Session.MultiCriteria and FlushMode.Auto inside transaction #865
Fixes NH-3350 - Duplicate records using Future() #1293
Fixes partially NH-3835 - Future/MultiCriteria 2nd level caching #840 (multi-criteria/query not fixed)
Fixes partially NH-3864 - Cacheable Multicriteria/Future'd query with aliased join throw exception #1344 (multi-criteria not fixed)

hazzik · 2018-06-01T10:19:53Z

I want you to confirm that you are OK in principle.

I'm OK.

fredericDelaporte · 2018-06-01T11:18:33Z

I am also ok on the principle, provided this is not overly complex to achieve. (I have not looked yet at how you do that.)

bahusoid · 2018-06-03T19:33:21Z

@hazzik , @fredericDelaporte Could you please review my changes in current state.

I got rid of auto-flushing in AbstractQueryImpl.GetTranslators. Really strange and unexpected place - retrieving query metadata should not lead to flush. And why to do check for each query in batch - it should be done once per batch execution. I also fixed #865

I've enabled batcher for stateless session. Seems to be working fine.

Should I obsolete existing future batcher with all related classes?

hazzik · 2018-06-04T10:59:15Z

@bahusoid I just wanted to suggest to make additions as a separate commits so it would be easier to see what's changed.

bahusoid · 2018-06-04T15:02:44Z

In fact IsUnifiedFuture setting has to be kept

Actually I don't think we need it.

We expose old future as:

nhibernate-core/src/NHibernate/Engine/ISessionImplementor.cs

Lines 329 to 331 in 2345bb4

    
           FutureCriteriaBatch FutureCriteriaBatch { get; } 
        
           FutureQueryBatch FutureQueryBatch { get; }

Where both FutureCriteriaBatch and FutureQueryBatch hard-wired to SessionImpl class via protected base ctor:

nhibernate-core/src/NHibernate/Impl/FutureBatch.cs

Lines 26 to 29 in 2345bb4

    
           protected FutureBatch(SessionImpl session) 
        
           { 
        
           	this.session = session; 
        
           }

And that's why old future is disabled for stateless session.

So we shouldn't bother to fallback to old code - old code can work only with SessionImpl.

bahusoid · 2018-06-04T15:19:11Z

I just wanted to suggest to make additions as a separate commits
A separated commit for separated subjects would be nice too

Ok. I will rewrite history in few commits. All changes are specific for this feature (and multi criteria fix is trivial after required for this change refactoring).

Enabling batcher for stateless session would also better have its own commit.

As already said in previous comment - it's supported only by new batcher.

src/NHibernate/Async/Impl/AbstractQueryImpl.cs

fredericDelaporte · 2018-06-04T13:25:14Z

src/NHibernate/Async/Impl/AbstractQueryImpl2.cs

@@ -116,15 +116,5 @@ public abstract partial class AbstractQueryImpl2 : AbstractQueryImpl
 				After();
 			}
 		}
-
-		protected internal override async Task<IEnumerable<ITranslator>> GetTranslatorsAsync(ISessionImplementor sessionImplementor, QueryParameters queryParameters, CancellationToken cancellationToken)
-		{


Same as previous comment.

fredericDelaporte · 2018-06-04T13:27:39Z

src/NHibernate/Async/Impl/SqlQueryImpl.cs

@@ -105,21 +105,5 @@ public override Task<IEnumerable<T>> EnumerableAsync<T>(CancellationToken cancel
 				After();
 			}
 		}
-
-		protected internal override Task<IEnumerable<ITranslator>> GetTranslatorsAsync(ISessionImplementor sessionImplementor, QueryParameters queryParameters, CancellationToken cancellationToken)


Same as previous comment.

fredericDelaporte · 2018-06-04T14:53:23Z

src/NHibernate/Engine/ISessionImplementor.cs

@@ -35,6 +35,11 @@ internal static IDisposable BeginProcess(this ISessionImplementor session)
 				// breaking change in case in custom session implementation is used.
 				new SessionIdLoggingContext(session.SessionId);
 		}
+
+		internal static IMultiAnyQueryBatch GetFutureMultiBatch(this ISessionImplementor session)


If obsoleting the previous batcher, better name the new one even more generically. Something like GetFutureBatch here.

fredericDelaporte · 2018-06-04T14:55:49Z

src/NHibernate/Impl/AbstractQueryImpl.cs

@@ -903,12 +903,20 @@ public IQuery SetResultTransformer(IResultTransformer transformer)

 		public IFutureEnumerable<T> Future<T>()
 		{
+			if (FutureSettings.IsUnifiedFuture)


As said in a standalone comment, this should be instead some checking on the session capabilities (a type test indeed), likely done in the constructor of the query class.

fredericDelaporte · 2018-06-04T16:11:54Z

src/NHibernate/Multi/MultiAnyQueryBase.cs

+	/// <summary>
+	/// Base class for both ICriteria and IQuery queries
+	/// </summary>
+	public abstract partial class MultiAnyQueryBase<TResult> : IMultiAnyQuery, IMultiAnyQuery<TResult>


IMultiAnyQuery already comes with IMultiAnyQuery<TResult>, reduce redundancy. An same remark than previously for naming.

fredericDelaporte · 2018-06-04T16:12:23Z

src/NHibernate/Multi/MultiAnyQuery.cs

+
+namespace NHibernate
+{
+	public partial class MultiAnyQuery<TResult> : MultiAnyQueryBase<TResult>, IMultiAnyQuery<TResult>


Same remarks as previously on naming and interfaces.

fredericDelaporte · 2018-06-04T16:22:37Z

src/NHibernate/Multi/MultiAnyQueryBase.cs

+			var count = _queryInfos.Count;
+			NewArray(count, out _hydratedObjects);
+			NewArray(count, out _subselectResultKeys);
+			NewArray(count, out _loaderResults);


Why not just writing regular code here?

_hydratedObjects = new List<object>[count]; _subselectResultKeys = new List<EntityKey[]>[count]; _loaderResults = new IList[count];

Adding a general purpose NewArray in that class just for this need here looks as a code smell to me. Either remove it or put that as an internal helper on a class like ArrayHelper.

fredericDelaporte · 2018-06-04T16:28:35Z

src/NHibernate/Multi/MultiAnyQueryBase.cs

+					return rowCount;
+				};
+
+				_loaderResults[index] = tmpResults;


Putting this after the yield leaves the state a bit inconsistent between each MoveNext when enumerating. Better move that before the yield.

fredericDelaporte · 2018-06-04T16:31:15Z

src/NHibernate/Multi/MultiAnyQueryBatch.cs

+	/// <summary>
+	/// Universal query batcher
+	/// </summary>
+	public partial class MultiAnyQueryBatch : IMultiAnyQueryBatch


Again, same remark for naming than on the interface.

bahusoid · 2018-06-05T05:47:24Z

I've split commit with some squashed clean up in tests.

Regarding naming for IMultiAnyQueryBatch, IMultiAnyQuery and related classes. We should choose new best pair :)

IQueryBatch and IAnyQuery
IAnyQueryBatch and IAnyQuery
IQueryBatch and IQueryBatchItem
Your suggestion

bahusoid · 2018-06-05T06:33:01Z

src/NHibernate/Linq/DefaultQueryProvider.cs

+	public interface INhQueryProviderSupportMultiBatch
+	{
+		IQuery GetPreparedQuery(Expression exrpession, out NhLinqExpression expression);
+		IMultiAnyQueryBatch GetFutureMultiBatch();


I'm thinking to expose ISessionImplementor Session instead. And get batch from session. Any objections?

I do not really see a benefit in exposing the whole session from the Linq provider. I would rather keep just exposing the future batch. Or even cease exposing it and add methods for adding new Linq query to it, a bit like the old interface was doing.

The benefit is to make further modifications simpler. If session was available and old future was using it - we wouldn't need to introduce new interface. It was only internal modifications in extension methods. Is Session really internal implementation detail for provider? I guess it can be abstracted one more layer down - but it looks to me as very rare breaking change (and such modification will be breaking in many more places).

Ok, go for it.

bahusoid · 2018-06-05T15:47:38Z

Will it be a big change to make them use the same query batcher than futures, allowing this way to only keep one non-obsoleted batcher? And it would also reduce the amount of code in which we have bugs to fix!

I was going to look in to it when new batcher is ready. But I suspect - yes. Are you OK with using new batcher cache approach in MultiCriteria\MultiQuery?

fredericDelaporte · 2018-06-05T16:00:35Z

Better have a single way of tackling the same subject everywhere, but let see that in another PR.

fredericDelaporte · 2018-06-05T16:05:59Z

src/NHibernate/Linq/LinqExtensionMethods.cs

@@ -2419,14 +2425,15 @@ public static IFutureEnumerable<TSource> ToFuture<TSource>(this IQueryable<TSour
 		/// <exception cref="T:System.NotSupportedException"><paramref name="source" /> <see cref="IQueryable.Provider"/> is not a <see cref="INhQueryProvider"/>.</exception>
 		public static IFutureValue<TSource> ToFutureValue<TSource>(this IQueryable<TSource> source)
 		{
-			if (FutureSettings.IsUnifiedFuture)
+			if(source.Provider is INhQueryProviderSupportMultiBatch batchProvider)


Missing space after if.

fredericDelaporte · 2018-06-05T16:06:02Z

src/NHibernate/Linq/LinqExtensionMethods.cs

@@ -2404,8 +2404,14 @@ public static Task<List<TSource>> ToListAsync<TSource>(this IQueryable<TSource>
 		/// <exception cref="T:System.NotSupportedException"><paramref name="source" /> <see cref="IQueryable.Provider"/> is not a <see cref="INhQueryProvider"/>.</exception>
 		public static IFutureEnumerable<TSource> ToFuture<TSource>(this IQueryable<TSource> source)
 		{
+			if(source.Provider is INhQueryProviderSupportMultiBatch batchProvider)


Missing space after if.

fredericDelaporte · 2018-06-05T16:11:50Z

src/NHibernate/Linq/DefaultQueryProvider.cs

+	public interface INhQueryProviderSupportMultiBatch
+	{
+		IQuery GetPreparedQuery(Expression exrpession, out NhLinqExpression expression);
+		IMultiAnyQueryBatch GetFutureMultiBatch();


I do not really see a benefit in exposing the whole session from the Linq provider. I would rather keep just exposing the future batch. Or even cease exposing it and add methods for adding new Linq query to it, a bit like the old interface was doing.

bahusoid · 2018-06-06T07:35:00Z

Looking at existing Future issues:

NH-3350 - Duplicate records using Future() #1293 - NH-3350 - Duplicate records using Future()
To fix it Loader.InitializeEntitiesAndCollections needs to be called right after query is loaded.
Fixed in 1ac0ad5
NH-3835 - Future/MultiCriteria 2nd level caching #840 - NH-3835 - Future/MultiCriteria 2nd level caching
Fixed for Future.
NH-3334 - 'collection is not associated with any session' upon refreshing objects from QueryOver<>().Future<>() #1290 - NH-3334 - 'collection is not associated with any session' upon refreshing objects from QueryOver<>().Future<>()
To fix it Session.PersistenceContext.InitializeNonLazyCollections(); needs to be called (in QueryBatchItemBase.PostProcess or maybe once per batch)
Not sure if it's a safe change. As before - all eager mappings were loaded lazly by Future. Wasn't it intentional behavior?
NH-2901 - Multiple .Fetch() calls within Future query results in collection that does not persist deletes #1249 - NH-2901 - Multiple .Fetch() calls within Future query results in collection that does not persist deletes
Unclear ticket. He either tries to fetch multiple bags in one query which throws exception (maybe missing delete due to wrong exception handling?)
Or tries to fetch not mapped collection - which is no op and doesn't affect behavior in any way (double checked it).
NH-3541 - Future queries of Criteria API/QueryOver are batched separately from other query methods #752 - NH-3541 - Multiple Queries of Criteria API/QueryOver are batched separately from other query methods
Fixed
NH-3864 - Cacheable Multicriteria/Future'd query with aliased join throw exception #1344 NH-3864 - Cacheable Multicriteria/Future'd query with aliased join throw exception
Fixed for Future

hazzik · 2018-06-20T02:26:12Z

Is this done? Or more work required apart from squashing commits?

fredericDelaporte · 2018-06-20T11:49:27Z

From my point of view, yes. But since the last commit is some refactoring I have made, I was expecting a feedback from @bahusoid, before approving.

I am already thinking of a follow-up PR which would obsolete the current multi-query/criteria for the query batcher introduced here. I do not think it should be done directly here, because this PR is already quite big, and its main subject was futures.

bahusoid · 2018-06-20T12:31:49Z

@fredericDelaporte I have no objections with your latest changes. And I also think it's ready.

Fixes nhibernate#752 Fixes nhibernate#1293 Fixes partially nhibernate#840 (multi-criteria/query not fixed) Fixes partially nhibernate#1344 (multi-criteria not fixed)

Fixes nhibernate#865

See nhibernate#1344 & nhibernate#1290

fredericDelaporte · 2018-06-23T13:17:24Z

Rebased and squashed.

fredericDelaporte

During my work on #1788, #1783 and #383, I realized that many points were wrong in the QueryBatcher initial implementation.
#1788 was fixing some of them by the way, but I think that we instead need a dedicated PR for fixing them.

src/NHibernate/Multi/QueryBatch.cs

+					return;
+				}
+
+				using (Session.BeginProcess())


src/NHibernate/Multi/QueryBatch.cs

+		protected void DoExecute()
+		{
+			var resultSetsCommand = Session.Factory.ConnectionProvider.Driver.GetResultSetsCommand(Session);
+			CombineQueries(resultSetsCommand);


src/NHibernate/Multi/QueryBatch.cs

+			int rowCount = 0;
+			try
+			{
+				if (resultSetsCommand.HasQueries)
