implementing FetchMode.SubSelect per query, and improving it

[nadav s]

hello, following the jira i've opened that explains the new feature: NH-2316

i've started working on it to hopefully upload a patch in the near future.

few issues with the current implementation of subselect fetching:

1. Its available only in the mappings, and not per criteria\hql query\linq query\query over

2. It isn't overrideable with fetch mode lazy. the reason is that it is not set for a specific query, but the persister of the collection, which is set as fetch="subselect", is some special persister for sub select fetched collections.

3. the query that is issued because of the subselect is very in-efficient. if the first query is:

from Foo f 

where f.Country.Region.Name = 'CoolRegion'

then the sub query created for foo.Children will be:

from Child c

where c.Foo.Id in

   (select f.Id

    from Foo f

    where f.Country.Region.Name = 'CoolRegion')

notice that if the query on foo has joins\sub selects then the query that gets foo's children is very inefficient, because after the query on foo was executed

the ids of foo are available so the query could easily be 

from Child c

where c.Foo.Id in (?,?,?...?

where the ? are the ids of the foos.

the 1 and 2 issues, make the subselect fetch unusuable when there are 2 use cases, one where lazy is appropriate and one where subselect fetching is appropriate

first of all, i'll be hapy to get your input about this. 

fixing issue 3 was suprisingly easy,

i've just replaced the code in NHibernate.Engine.SubselectFetch that used to take the query string of the original query to be used for the sub select

and replaced it with this code:

public SqlString ToSubselectString(string ukname)

        {

            SqlStringBuilder sqlBuilder = new SqlStringBuilder();

            for (int i = 0; i < this.resultingEntityKeys.Count; i++)

            {

                if (i != 0)

                {

                    sqlBuilder.Add(",");

                }

                sqlBuilder.AddParameter();

            }

            SqlString sqlString = sqlBuilder.ToSqlString();

            return sqlString;

        }

of course i also had to set the query parameters for the ids.

taking the ids of the original query was easy because they are already there (in this.Results)

all unit tests pass so i think its ok.

about issues 1 and 2 - enabling a FetchMode.SubSelect, and in general, making subselect a fetch mode just like join - i wanna start working on that

but first would like to know that you think all of this is a good idea and if you have pointers on how to do it - because

for criteria its obvious - just use the FetchMode, for Linq its obvious - there needs to be an extention method but for hql - should it be a methid of IQuery or 

should it be some token in the HQL?

for conclusion, i think that once implemented like that, it will be very very easy to fetch a whole object graph for a paged query on the aggregate root.

thanks in advance.

Nadav.

[Fabio Maulo]

what you are looking for is not subselect but batch-size.

The batch-size will use IDs of collection-owner loaded in the session.

--
Fabio Maulo

[nadav s]

thanks for the response fabio.

few questions:

1. as far as i can see this is also something that is set via mappings and cannot be set/overriden for a specific query

2. if this is batch size what's the fetch="subselect" for?

3. it might be impossible to know the required batch size, if the query isn't paged but just filtered with some filter on the entity, and the collections should be fetched for the whole results of the first query

so first of all, i think the improvement of the fetch="subselect" is good for performance in any case don't you think?

should i try to focus on enabling batch-size to be set for a specific query instead of focusing on sub select being set for a specific query? do you agree that it could  be a nice feature?

thanks.

[Fabio Maulo]

1. It is wrote in the mapping but you can eager fetch a collection in a specific query

2. as you saw "subselect" has a different behavior, for that reason batch-size is needed

3. I can't understand the sense of your question. batch-size=500 will upload at most 500 collections but if the session has only 25 owner it will load 25 collections. If in the session you have 900 owners, NH will load 500 collections first then when you will try to access to the collection of owner 501, and only if you will try it, NH will load the next batch of 400 collections. (Note: if you have 900 collection-owner in the session, your problem is not the performance).

subselect may cause more problems than it solves (you know it).

In my strictly personal opinion, subselect should be removed (as should be removed composite-id and its friends).

--
Fabio Maulo

[nadav s]

ok i get your point.

but:

1. It can be overriden with eager loading, but not with regular lazy loading

so if you have two use cases, one that loads all fathers and accesses one or two of their children, and another that want to batch load the children. setting batch size in the mappings don't give full control over batching because it can't be changed for a specific query.

do you think it will be a nice feature to enable overriding the batch size for a sepecific query, with lazy fetching, and also

having lazy fetching as default and enabling, per query, set the batch size for a collection and a collection's collection?

if not, i'll let it go, i just know we had some issues with selecting a whole graph, but not all the time, and subselect was impossible to use, and batch size gave good but not optimistic performance because it isn't overidable with fetch lazy and vise versa

[Fabio Maulo]

are you talking about a WEB app or a rich-client app ?

--
Fabio Maulo

[Diego Mijelshon]

Being able to override batch-size would be useful. Implementing it requires messing with more than one part of the infrastructure, though.
 
    Diego

[Fabio Maulo]

It is possible for batcher (INSERT, UPDATE,DELETE).

I don't understand where it is useful for collection/relations batch-size.

--
Fabio Maulo

[Diego Mijelshon]

I have entities where batch loading helps in some use cases but it loads lots of unneeded entities/collections in other complex use cases, where I have many proxies but only use a few.

My current workaround is doing "manual batch loading" (i.e. dummy query) in the cases where I need it.

It would be definitely a low-priority but nice-to-have feature.
 
    Diego

[nadav s]

i don't think its thats low priority, because it is actually a thing people expect to happen when they set a fetch mode to Eager, at least i've seen alot of situations when people really thought that thats whats going to happen  (later finding out it killed their query with CP)

about when it is helpful - exactly in the situations diego described. two use cases, 

in one of them you query the fathers and gonna need only one of the father's collection, and for the other

you're gonna need all of their collections.

it gets more complicated when there are grandchildren involved, and in one of the situations you want the grand children of one of the childs, and in the other situation, because you load an object graph, you're gonna need all of them.

now, either you implement (similar to what diego said) the loading of the collections yourself, or you gonna have to live with the batch size slowing down the first situation, where you would have prefered lazy loading without batching

[Fabio Maulo]

real use-case and environment (rich-client or web), please.

--
Fabio Maulo

[nadav s]

btw, i don't really get what is the problem with subselect, as it lets you efficiently fetch a whole object graph for the N fathers that were fetched in some query, in the most efficient way possible

[Fabio Maulo]

I don't know which is the problem... you said that there is a problem and you want change it using the same tech used by batch-size (using uploaded ids) because subselect seems inefficient in some cases. 

--
Fabio Maulo

[nadav s]

the sub select is always inefficient, especially when there is an initial complex query (with sub queries in it), and its a killer when its a two level tree (when fetching the grandchildren). fixing it was really really easy, and i can't see any downside to it.

different use cases in a web app:

use case 1: sub select\batch size is NOT desired

   the user searches for car companies by some criteria. the user will then choose (double click on a grid's row or something) one of the     

   companies to see it in full details. each company has one-to-many car types (mazda -> mazda 3, mazda 5, mazda 6...) and each 

   car type will be displayed in its own tab, when at first, the newest car type or the most expensive one, doesn't matter is selected.

   each car type has its models, mazda3 2008 isn't the same as 2010 (i don't that much about cars and not sure the years are correct, 

   but there are differences between the models). 

   the result: if carType.Models is mapped with some batch size, say 10, the models of 10 of the car types are now fetched, although 

   the user only watches the models of one of the car types, if there could be lots of models for each car type, it slowed the first tab, 

   and made the other tabs faster, because their car types are now loaded, but its not what is desired, because the user is expected to 

   click on only one of other tabs or something.

 use case 2: desired:

    the user wanna see some custom developed report (ui that can be implemented with MRS/Cognus or any other reporting framework,   

    and we have all kinds of reports that live up to this definition, and for some good reasons also). for the report the user searches for 

    car companies by some criteria (some search form) and then expects to see the returned companies, paged of course, but with all 

    of their car types, and for each of the car type - all of its models. here, a sub select or batch fetching is a must or else we'll get a CP 

    with join fetching, or N^2 + 1 if we do regular lazy loading (like we wanted to do in the first situation).

of course we can work around that, and thats exactly what we do, using a generic mechanizm that for reports, eager fetches with sub selects and not joins, the association it was asked to fetch. for the regular queries, it just use the default which is regular lazy.

it would have been really really nice, if i could have set, for the report query, query.SetFetchMode("CarTypes", FetchMode.SubSelect)

or if you will, query.SetBatchSize("CarTypes", 20)

and same for models

query.SetFetchMode("CarTypes.Models", FetchMode.SubSelect) or 

query.SetBatchSize("CarTypes.Models", int.MaxValue).

it must be max value because i want all the models, and can't possibly know how many car types are going to be there. of course it won't be alot, because the "query" is going to use paging, but i don't really know if its 20, 40, or something else.

batch size, currently makes me choose between the use cases, slowing down one of them, or makes me query and connect the associations my self. same goes for sub select, which also issues an inefficient query for CarTypes and a killer query for the Models

before my fix it would have been:

select ...

from Models m

where m.CarTypeId in

   (select c.Id

    from CarTypes c

    where c.CompanyId in

            (select company.Id

             from Companies company

             where <could be some crazy crteria - this is the same where clause of the very original query>))

(i was able to make itthe inefficiency of the query 

[Fabio Maulo]

LOL!!

Your first assertion : "btw, i don't really get what is the problem with subselect"

Your second assertion : "the sub select is always inefficient"

--
Fabio Maulo

[Fabio Maulo]

The batch-size can't be set in a query because it will be not applied in the query; it will be applied if/when you will access to the collection.

--
Fabio Maulo

[John Davidson]

I think nadav is saying that subselect from NHibernate is an issue, but the implementation he is proposing will fix that problem

John Davidson

[nadav s]

no you didn't understand me.

the current implementation of subselect is always inefficient, and some times a real killer. on the other hand, after my fix (which you say makes the subselect be the same as the batch size) it is efficient.

i've meant i don't see the problem with the concept of fetch="subselect". i think the concept is ok, and after my fix, it is efficient. all that is left to do is being able to override it with lazy and override lazy with subselect for a specific query. or at least make the batch size overrideable

[Fabio Maulo]

aaaahhhh ok.

Now it is a little bit more clear.

Ok for the patch when you will send it.

Then we will talk about fetch="batch" : the same but with one roundtrip using the same we are doing with multi-query

--
Fabio Maulo

[nadav s]

you know the internal of nhibernate much much much better than me, and i won't get into an implementation argue with you, but it is possible to implement.

with subselect (again i'm talking about subselect because i didn't do any research on the batch size, but i guess the idea is similar because it works the same, only batch size issues a good query and subselect issues an evil one), as i've noticed, there is a special one-to-many collection persister, that knows once the collection is accessed, use a sub select batcher that loads the collections of all the owners that were returned by the initial query.

if the persister could have been set, or modified, for a specific instance of a collection, it would have been possible - you could have set the batch size\subselect for a specific query, which in turn would have set a different persister for the collections that their persisters needs modification, and then when a collection would have been accessed, the persister would have done its thing.

of course, i'm not sure thats the proper way of implementing it, but as an idea - tell the specific collections that are created for the entities of a specific query to do something else than the default, it is possible

[nadav s]

great. thanks

[Fabio Maulo]

The matter is that, after load owner, with a specific query, if you want know all IDs loaded you will end to do exactly the same thing done using batch-size (you will look to the owners already uploaded in the session).

After do that the next problem will be: ok, I have uploaded 500 owner but I don't want upload all its collections in one shot.... blah... blah... and you will end in the actual work done by "batch-size".

If you want do that work case-by-case, you will need a way to specify the behavior UoW-by-UoW and collection-per-collection... if you can see another solution I'll be happy to know it

--
Fabio Maulo

[Fabio Maulo]

ah...

take care with "using IDs is more efficient" because "subselect" does not suffer the problem of max-parameter (IIRC 2100 in msSQL)

--
Fabio Maulo

[nadav s]

of course i'll be doing the same work as batch size, i'm not set to implement batch size all over again, but trying to allow it to be query specific, meaning, being able to look for owners of a specific query, and not all owners that are in the session (owners from different query might be there), and allowing it to be overrideable

[nadav s]

before starting to work on the batch size thing, i want to apply a patch for making subselect more sain because that work is already done, the problem is, in the old way, if  the first query used paging, the sub select ignored the paging, fetching all the children of the fathers of the first query, if it hadn't used paging.

there is a test called SubselectFetchWithLimit which creates 3 parents, fetches only 2 parents, initiliaze their collections, then fetches the third parent and expects its collection to already by initialized (although it wasn't returned by the paged query).

my improvement breaks this test, because paging is now taking into consideration when fetching by ids, which i think is the much more correct way to go.

so the assert with the comment

// The test for True is the test of H3.2

now breaks. can i change the test intentially so that subselect will consider paging?

[Fabio Maulo]

Yes you can, but I think you are introducing a breaking change if you are going for:

where c.FooId in (p1, p2, p3, p4..... pn)

Subselect does not suffer the 2100 parameters limit, with your patch it will have this new limitation.

--
Fabio Maulo

[Diego Mijelshon]

If you do implement a customizable batch-size, it should be per-session...

Something like the existing SetBatchSize and EnableFilter...

A possible API:

session.EnableBatching(typeof(Foo), 100); //on Foo entity

session.EnableBatching(typeof(Foo), "Bars", 50); //on the Foo.Bars collection

session.EnableBatching("Foo.Bars", 50); //alternative

session.DisableBatching(...); //with the same options
 
    Diego

[nadav s]

about the subselect breaking changes: yeah i got that, and its un resolveable. i think its better this way because i just think it had alot of performance issues previously, but with all do respect to my self, i can't be the one to make this decision :)

about the per session batching, seems much more reasonable and much easier than per query

[Frans Bouma]

> Yes you can, but I think you are introducing a breaking change if you are
> going for:
> where c.FooId in (p1, p2, p3, p4..... pn) Subselect does not suffer the
2100
> parameters limit, with your patch it will have this new limitation.

Yep, paging is a problem. We have the same feature, called
'parameterized prefetch paths', which works roughly like this: you have a
threshold T which is say by default 100. If there are < T parentID's, it
switches to a where x.FkField in (p1, p...) query for children, and if >= T,
it will use a subquery.

Paging however doesn't work with a subquery, you have to use the
parameterized variant with eager loading. This isn't that hard to achieve
however: just keep the page size < T.

In NH land, the 'T' is the batchsize, but it's not configurable at
runtime, correct? So you can't set it per query. It depends whether a
subquery is faster than a parameterized variant. Speed tests on sqlserver
suggest that when you hit 100 or more parameters, a subquery is quicker, but
complex subqueries could make this number become different, so it is highly
useful to have a configurable batch size.

FB

[Fabio Maulo]

Not enough... it should allow the batch-size per collection (property-path) not only per-class.

--
Fabio Maulo

[Fabio Maulo]

Ups sorry you have already included the option for collection...

As you can see, nadav, the work to do in each UoW will be pretty ugly than the batch-size by default in the mapping...

I would see how you will manage it in your application where, hopefully, the session is managed in a generic way not depending on a specific use-case.

--
Fabio Maulo

[nadav s]

its much more pretty that what needs to be done now to achive the same result

[nadav s]

NH-2316

although the name of the jira is subselect, the actual patch lets a developer set the batch size for an entity\collection overriding the settings in the mappings.

looking forward for comments\application of the patch.

thanks alot.

[Diego Mijelshon]

Nice job!

How about a strongly-typed version of the collection ones?

  void SetFetchingBatchSize<TEntity>(Expression<Func<TEntity, IEnumerable>> association, int batchSize); 

Allowing:

  session.SetFetchingBatchSize<Father>(f => f.Children, 5);
 
    Diego

[nadav s]

done but i'm not sure i did it correctly as i never had to proccess a linq expression...

is this right?

  public void SetFetchingBatchSize<TEntity>(Expression<Func<TEntity, object>> association, int batchSize)

        {

            if (!(association is LambdaExpression) || !(association.Body is MemberExpression))

            {

                throw new ArgumentException("Batch size can only be set by a lambda expression which returns a member association");

            }

            this.SetFetchingBatchSize<TEntity>(ExpressionProcessor.FindMemberExpression(association.Body), batchSize);

[nadav s]

changed the object to ienumerable, still wondring about the argument exception there, weather this is correct

[Frans Bouma]

Beware of IEnumerable: 'string' also implements IEnumerable, so
c=>c.CompanyName will also match, while it's not a collection ;)

FB

> NH-2316 <http://216.121.112.228/browse/NH-2316>

[nadav s]

well an exception will be thrown because a collection persister for company name won't be found

[Wenig, Stefan]

Right. Still, if you like to nail stuff down statically, you could do this:

 

public void SetFetchingBatchSize<TEntity, TTargetEntity>(Expression<Func<TEntity, IEnumerable<TTargetEntity>>> association, int batchSize)

where TEntity: class, TTargetEntity: class

[nadav s]

fine by me as i always use generic collections, but isn't it possible to map an association with non generic collection?

btw, won't it be a problem with a map? IDictionary<int,Child> is not IEnumerable<Child>

[Wenig, Stefan]

Don’t know, I’m just talking C# here. If you have a OrderItemsCollection type that does not implement IEnumerable<OderItem>, you absolutely need to support IEnumerable, period. I’d rather require the interface, but that’s just me and you have existing code to consider.

 

IDictionary<TK,TV> would need a separate overload, because while it implements IEnumerable<KeyValuePair<TK,TV>>, KeyValuePair is a struct.

 

That said, you might be better off just accepting the runtime exception for strings. Your call.

 

Cheers,

Stefan

[Diego Mijelshon]

You are right.

Since the validation is useless, you can just use Func<T, object>, like ConfORM
 
    Diego

[Diego Mijelshon]

In ConfORM:

		public static MemberInfo DecodeMemberAccessExpression<TEntity>(Expression<Func<TEntity, object>> expression)

		{
			if (expression.Body.NodeType != ExpressionType.MemberAccess)

			{
				if ((expression.Body.NodeType == ExpressionType.Convert) && (expression.Body.Type == typeof(object)))

				{
					return ((MemberExpression)((UnaryExpression)expression.Body).Operand).Member;

				}
				throw new Exception(string.Format("Invalid expression type: Expected ExpressionType.MemberAccess, Found {0}",

									expression.Body.NodeType));
			}
			return ((MemberExpression)expression.Body).Member;

		}

 
    Diego

[Wenig, Stefan]

I don’t see how catching _some_ errors statically is „useless”. The constraint for non-generic IEnumerable is more expressive too, and costs nil.

 

From: nhibernate-...@googlegroups.com [mailto:nhibernate-...@googlegroups.com] On Behalf Of Diego Mijelshon
Sent: Friday, September 03, 2010 1:41 PM
To: nhibernate-...@googlegroups.com
Subject: Re: [nhibernate-development] implementing FetchMode.SubSelect per query, and improving it

 

You are right.

[nadav s]

can you help me with the question i've asked?

does this seem ok? - the exception that i throw, i'm completely not sure about this

public void SetFetchingBatchSize<TEntity>(Expression<Func<TEntity, IEnumerable>> association, int batchSize)

        {

            if (!(association is LambdaExpression) || !(association.Body is MemberExpression))

            {

                throw new ArgumentException("Batch size can only be set by a lambda expression which returns a member association");

            }

            this.SetFetchingBatchSize<TEntity>(ExpressionProcessor.FindMemberExpression(association.Body), batchSize);

        }

about the static constraint, i think ienumerable is a fair choice as it covers all association, and who ever set a batch size on a string property, its kinda his fault and he deserves his runtime exception :)

[Diego Mijelshon]

There's one little problem: you can use non-generic collections with NHibernate, therefore you need to support IEnumerable.
 
    Diego

[Wenig, Stefan]

If you read carefully, I think we all agree on _non-generic_ IEnumerable by now (not object, not IEnumerable<T> either)

[nadav s]

yeah thats what i said and thats why its non generic ienumerable

[Diego Mijelshon]

That's fine
 
    Diego

[nadav s]

Sorry for may be being a pain in the ass, but is there any estimation on when could this be patched into the trunc?