Obsolete MultiQuery and MultiCriteria #1783

fredericDelaporte · 2018-07-05T14:13:12Z

The QueryBatch can replace them both, and does not suffer of some of their bugs.

This is a follow-up to #1718 (Combine Future queries for ICriteria and IQuery), which has added the QueryBatch.

At first I was considering adding to it the SetParameter capabilities of MultiQuery, so I was considering better to do that in another PR. But finally, I think we can drop that feature, as it does not make sense for most querying API handled: Criteria, QueryOver and Linq. Only HQL and SQL queries can benefit of it.

Closes #840 by obsoleting the not already fixed part of it.
Closes #1344 by obsoleting the not already fixed part of it.

doc/reference/modules/performance.xml

-            in a single round trip. A simple use case is executing a paged query while
-            also getting the total count of results, in a single round-trip. Here is a simple
-            example:
+            You can also add queries as future queries to a query batch:


fredericDelaporte · 2018-07-05T14:15:28Z

src/NHibernate.Test/Criteria/Lambda/IntegrationFixture.cs

@@ -340,7 +341,7 @@ public void FunctionsOrder()
 			}
 		}

-		[Test]
+		[Test, Obsolete]


For each obsoleted test, I have added a query batch counterpart.

fredericDelaporte · 2018-07-05T14:17:34Z

src/NHibernate.Test/QueryTest/QueryBatchFixture.cs

+	public class QueryBatchFixture : TestCase
+	{
+		// This fixture aggregates most of the tests from MultiCriteriaFixture, MultipleMixedQueriesFixture and
+		// MultipleQueriesFixture, rewritten for using QueryBatch instead of obsoleted MultiCriteria/MultiQuery.


The three obsoleted multi-query/criteria fixtures have their counterparts with query batch here. I have dropped some tests which were a bit redundant.

fredericDelaporte · 2018-07-05T14:20:29Z

src/NHibernate.Test/QueryTest/QueryBatchFixture.cs

+
+			Sfi.Statistics.Clear();
+			DoMultiCriteriaAndAssert();
+			Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(0), "Prepared statements");


The original test was doing something quite more elaborated, but brittle: it was hacking the cache to directly alter the cached representation in it with fake values, then it was re-executing the queries and checking it was receiving the fakes values.
Sure, that way, we are certain the cache has been hit, but it heavily relies on its current implementation, on the disassemble/assemble implementation, ... So I have replaced that by a statistics check.

fredericDelaporte · 2018-07-05T14:21:43Z

src/NHibernate.Test/Stats/StatsFixture.cs

 					var r = s.CreateMultiCriteria().Add(DetachedCriteria.For<Country>()).Add(DetachedCriteria.For<Continent>()).List();
+#pragma warning restore 618


This test does test many other things, so I have just disabled the obsolete warning for the multi-part. (And added a query batch counterpart below.)

To simplify removing obsoleted things later I think it's better scope this disable warning to the whole block that should be remove.

fredericDelaporte · 2018-07-05T14:26:48Z

src/NHibernate/Multi/QueryBatchItemBase.cs

 					if (queryParameters.HasAutoDiscoverScalarTypes)
 					{
-						loader.AutoDiscoverTypes(reader, queryParameters, null);
+						loader.AutoDiscoverTypes(reader, queryParameters, forcedResultTransformer);


The null value was originating from the MultiQuery implementation, which has a special cache handling, due to caching all queries together as a single entry. But for caching queries independently, this is a bug wrecking the AutoDiscoverTypes feature. Fortunately, one of the many tests added by rewriting multi-criteria/queries tests as query batch tests has allowed to spot this bug.

fredericDelaporte · 2018-07-06T10:07:31Z

Rebased and amended for accounting TestCase changes.

The QueryBatch can replace them both, and does not suffer of some of their bugs.

fredericDelaporte · 2018-07-11T22:44:52Z

Rebased for resolving conflicts with #1789.

bahusoid · 2018-07-14T06:04:41Z

src/NHibernate.Test/NHSpecificTest/NH2195/SQLiteMultiCriteriaTest.cs

+		{
+			var driver = Sfi.ConnectionProvider.Driver;
+			if (!driver.SupportsMultipleQueries)
+				Assert.Ignore("Driver {0} does not support multi-queries", driver.GetType().FullName);


Are you sure we want to skip such tests (that are not checking that single query is executed for batch but some data conditions retrieval)? As query batch doesn't throw maybe it's good to test that it also works adequeatly when driver.SupportsMultipleQueries == false.

I have port existing multi-query/criteria tests without changing their conditions. Many of them would fail with drivers not supporting multiple queries, because they would see more queries executed than expected.

In fact none of them do check sql queries count, so these conditions were all removable. (Excepted one causing trouble to Firebird by using count as a column alias name, requiring changing the query.)

bahusoid · 2018-07-14T07:21:16Z

src/NHibernate.Test/QueryTest/QueryBatchFixture.cs

+		/// Get the inner Hashtable from the IQueryCache.Cache
+		/// </summary>
+		/// <returns></returns>
+		public static Hashtable GetHashTableUsedAsQueryCache(ISessionFactoryImplementor factory)


Could you please clarify the reasons for this hackish method? It seems all usages are equivalent to factory.QueryCache.Clear()

No I cannot. That is just the way it was done for multi-criteria/query.

In fact reading the code, I can: it is not used only for clearing, but also for checking directly that it gets filled with items.

Isn't it something that can be checked with "Sfi.Statistics.QueryCachePutCount"?

It could, assuming there are no bugs in stats. Here with this test it is really certain things are in the cache.

But that's something that you are already using for other tests - CanGetMultiCriteriaFromSecondLevelCache for example.
So the whole usage seems to be analog of clearing statistics and checking put counts. IMHO we should get rid of this method for good.

Yes I know it is done for other tests. About tests I do not think consistency in the way things are tested is good. Instead, testing with different ways help catching more cases.

I see your point. But for me it looks like introducing dirty hack for no particular reasons (to me checking count of items in internal cache implementation doesn't add any value to existing tests - you can't say for sure that those items are stored properly and can be retrieved by the same query reexecution).

Enable tests also for driver not handling multiple queries

Remove some reflection based check

bahusoid · 2018-07-16T09:17:51Z

doc/reference/modules/performance.xml

+var queries = s.CreateQueryBatch()
+    .Add("queryOverList", s.QueryOver<Item>().Where(i => i.Category == "Food"))
+    .Add<long>("sqlCount",
+        s.CreateSQLQuery("select count(*) as count from Item i where i.Category = :cat")


As this "as count" can cause issues on some dialects maybe we shouldn't use it documentation.

bahusoid · 2018-07-16T09:33:50Z

src/NHibernate.Test/NHSpecificTest/NH2201/Fixture.cs

+		{
+			using (var s = OpenSession())
+			{
+				Console.WriteLine("*** start");


I think it's better to remove these "*** star*" and "*** end" logging in ported tests.

bahusoid · 2018-07-16T09:38:23Z

src/NHibernate.Test/Stats/StatsFixture.cs

 					var r = s.CreateMultiCriteria().Add(DetachedCriteria.For<Country>()).Add(DetachedCriteria.For<Continent>()).List();
+#pragma warning restore 618


To simplify removing obsoleted things later I think it's better scope this disable warning to the whole block that should be remove.

Add some more adjustments according to review

bahusoid

LGTM

fredericDelaporte added c: Core p: Minor t: Task labels Jul 5, 2018

fredericDelaporte added this to the 5.2 milestone Jul 5, 2018

fredericDelaporte commented Jul 5, 2018

View reviewed changes

fredericDelaporte force-pushed the ObsoleteMultiQuery branch from 79abe83 to d70038b Compare July 6, 2018 10:05

This was referenced Jul 9, 2018

NH-3596 - DistinctRootEntityResultTransformer breaks Cache #383

Closed

Combine Future queries for ICriteria and IQuery #1718

Merged

[WIP] Cacheable Multicriteria/Future'd query with aliased join throw exception #1733

Closed

Obsolete MultiQuery and MultiCriteria

8cc516b

The QueryBatch can replace them both, and does not suffer of some of their bugs.

fredericDelaporte force-pushed the ObsoleteMultiQuery branch from f646fc9 to 8cc516b Compare July 11, 2018 22:44

bahusoid reviewed Jul 14, 2018

View reviewed changes

fredericDelaporte added 3 commits July 14, 2018 11:33

fixup! Obsolete MultiQuery and MultiCriteria

7e40130

Enable tests also for driver not handling multiple queries

Merge branch 'master' into ObsoleteMultiQuery

e8d4c5c

fixup! Obsolete MultiQuery and MultiCriteria

9f7d805

Remove some reflection based check

bahusoid reviewed Jul 16, 2018

View reviewed changes

fixup! Obsolete MultiQuery and MultiCriteria

b628962

Add some more adjustments according to review

bahusoid approved these changes Jul 17, 2018

View reviewed changes

fredericDelaporte merged commit 6e1c370 into nhibernate:master Jul 17, 2018

fredericDelaporte deleted the ObsoleteMultiQuery branch July 17, 2018 09:00

fredericDelaporte added the r: Fixed label Jul 17, 2018

This comment has been minimized.

Sign in to view

		var r = s.CreateMultiCriteria().Add(DetachedCriteria.For<Country>()).Add(DetachedCriteria.For<Continent>()).List();
		#pragma warning restore 618

Obsolete MultiQuery and MultiCriteria #1783

Obsolete MultiQuery and MultiCriteria #1783

Uh oh!

Conversation

fredericDelaporte commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Jul 6, 2018

Uh oh!

fredericDelaporte commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

fredericDelaporte commented Jul 5, 2018 •

edited

Loading