WIP - Add support for IAsyncEnumerable

[maca88]

Limitations: The added Linq extension method AsAsyncEnumerable do not support PostExecuteTransformer as the transformer operates with IEnumerable<>.

Fixes: #2267

Added WIP as the async generator has some issues generating obsolete overloads and tests with IAsyncEnumerable.

[hazzik]

Looks good so far.

[bahusoid]

The added Linq extension method AsAsyncEnumerable

I'm pretty sure that Enumerable.AsEnumerable LINQ extension leads to IQuery.List on iteration. That makes sync counterpart quite different from async version.

We either need to make AsEnumerable to use PerformIterate or maybe use different methods. Like ToEnumerable/ToAsyncEnumerable.

[maca88]

I'm pretty sure that Enumerable.AsEnumerable LINQ extension leads to IQuery.List on iteration

I tested it and calling AsEnumerable does not execute any query, which means that it is possible to iterate over the same enumerable multiple times. I've mistakenly added such test for it instead of IAsyncEnumerable: https://github.com/nhibernate/nhibernate-core/pull/2274/files#diff-64ae2936b1d2c700144c9d334ef6d9a6R12

[bahusoid]

I tested it and calling AsEnumerable does not execute any query,

Yeah that's expected (As suggests that it only casts inside ) I meant on actual enumeration IQuery.List will be called. AsAsyncEnumerable name suggests that it behaves as AsEnumerable but in async way. But it's not the case. It's completely different behavior - AsEnumerable ends up calling IQuery.ToList and AsAsyncEnumerable version calls IQuery.AsyncEnumerable.

It seems we currently don't expose LINQ method executing IQuery.Enumerable. But if we add one I don't think it should be executed on standard AsEnumerable extension - it should be some separate method (maybe ToEnumerable? and then async counterpart should be ToAsyncEnumerable).

[maca88]

AsAsyncEnumerable name suggests that it behaves as AsEnumerable but in async way. But it's not the case. It's completely different behavior.

In the current state of this PR they are not completely different, the only difference is that AsEnumerable will read all rows from the data reader when the first MoveNext is called, where AsAsyncEnumerable will read just one. Basically AsEnumerable works similar as a future query which executes the query and read all rows when first accessed, which kills the meaning of AsEnumerable. The issue lies in DefaultQueryProvider.ExecuteQuery method where IQuery.List is called instead of IQuery.Enumerable: https://github.com/nhibernate/nhibernate-core/blob/d8931ac97e97f56d811ea8b5383c08e39a96df40/src/NHibernate/Linq/DefaultQueryProvider.cs#L209 Using IQuery.List means that the rows will be traversed twice even when Enumerable.ToList() is called, first inside IQuery.List and the second time inside Enumerable.ToList(), which basically passes the result into the List constructor: https://github.com/microsoft/referencesource/blob/17b97365645da62cf8a49444d979f94a59bbb155/System.Core/System/Linq/Enumerable.cs#L947-L950

It seems we currently don't expose LINQ method executing IQuery.Enumerable

I think that we should instead change the current method (DefaultQueryProvider.ExecuteQuery) to use it instead of IQuery.List. By using IQuery.Enumerable, AsEnumerable would still not work as expected because IQuery.Enumerable does not support multiple iterations, so we would also need to change that. Do really IQuery.Enumerable needs to execute the query when called, is there a reason behind it?

[bahusoid]

Do really IQuery.Enumerable needs to execute the query when called, is there a reason behind it?

Did you mean IQuery.List? Well as far as I know ISession.Enumerable is not implemented for stateless session (don't know why)

I think that we should instead change the current method (DefaultQueryProvider.ExecuteQuery) to use it instead of IQuery.List.

You are right. Seems like a proper way. We should fix the way AsEnumerable works instead of introducing separate methods (not in this PR of course). So just ignore my renaming suggestions.

[maca88]

Did you mean IQuery.List?

No, I meant the following code that is executed when IQuery.Enumerable is called: https://github.com/nhibernate/nhibernate-core/blob/d8931ac97e97f56d811ea8b5383c08e39a96df40/src/NHibernate/Loader/Hql/QueryLoader.cs#L478-L481 which execute the query, preventing it from iterating the enumerable multiple times:

var enumerable = s.CreateQuery("from Customer").Enumerable<Customer>();
enumerable.ToList();
enumerable.ToList();

Well as far as I know ISession.Enumerable is not implemented for stateless session (don't know why)

Good point, that was probably the reason why List was used as it supports more features than Enumerable method. It was probably not implemented because IQueryPlan.PerformIterate has an IEventSource parameter and stateless session does not implement it. The IEventSource is used by EnumerableImpl which uses ISession.DefaultReadOnly (IEventSource extends ISession) property that is not available with a stateless session. In order to support it for the stateless session we would need to change the parameter type to ISessionImplementor and then inside EnumerableImpl use the DefaultReadOnly only when the given session is castable to ISession.

[hazzik]

So, with the latest iteration of changes this has become a close resemblance of "Future" feature, right?

[maca88]

The last iteration of changes applied the same logic in terms of when the query is executed to the current Enumerable methods, which is similar how "Future" feature work, right.

[maca88]

Rebased and re-generated with AsyncGenerator 0.18.1

[maca88]

Only now I realized that PerformIterate and now PerformAsyncIterate trigger N+1 queries. For example the following query:

s.CreateQuery("from Simple").Enumerable<Simple>()

will trigger one query that will retrieve all ids and then on each MoveNext call a separate query will be triggered for the current id, if the entity is not in the cache (hibenate does the same). Hibernate has also scroll method that does what I thought Enumerable does, which executes only one query. For me, the behavior of scroll method is more suitable for Enumerable and AsyncEnumerable but it would require to port the feature from hibernate, which is a topic for a separate PR.

[maca88]

Rebased.

[bahusoid]

Only now I realized that PerformIterate and now PerformAsyncIterate trigger N+1 queries.

That sucks :( It's not very useful for entities but I guess it's still can be useful for scalar values...

For me, the behavior of scroll method is more suitable for Enumerable and AsyncEnumerable but it would require to port the feature from hibernate, which is a topic for a separate PR.

Yeah. And I would just replace Enumerable implementation with scroll. Current enumerable entity handling is not intuitive at all.

[maca88]

Yeah. And I would just replace Enumerable implementation with scroll

One issue that I didn't checked, is how scroll behave when the query has collection fetches. Hibernate uses FetchingScrollableResultsImpl, but I didn't tested it yet to understand how it works.

[fredericDelaporte]

So, with the latest iteration of changes this has become a close resemblance of "Future" feature, right?

The last iteration of changes applied the same logic in terms of when the query is executed to the current Enumerable methods, which is similar how "Future" feature work, right.

The main purpose of the Future feature is not deferred execution but batching together queries. (Future also features a very loose coupling between code parts defining the queries, contrary to the recent query batcher which requires referring to the same batcher reference for adding queries into it, but as such also helps knowing exactly what is batched together.) Deferred execution is just a side effect of how Future works.

This IAsyncEnumerable feature does nothing to batch queries together, so it is still two quite different features.

Anyway, it appears this "enumerable" feature is not very attractive with its current performances issues.

Is it the reason for this PR to be stalled? Or should its conflicts be resolved for allowing merging it?

[maca88]

Is it the reason for this PR to be stalled?

Yes, the scroll feature from Hibernate should be ported first, so that it can be used in this PR to avoid N+1 issue. I haven't started working on it yet as my current priority is the Include method. Changed to WIP until the scroll feature is ported.

[zlsmith86]

Is there an issue to track the porting of the scroll feature?

[Harpush]

Any progress here with the scroll feature?