Fix shortcomings of QueryBatcher initial implementation #1789
Conversation
* AutoFlush is handled too late, causing stale data to be yielded * Cacheable result transformation is not fully done * Auto discovery of types fails * InitializeEntitiesAndCollections may be wrongly skipped or called * DoExecute should be renamed ExecuteBatched * BeginProcess should span the whole processing Follow up to nhibernate#1718. See its reviews for more details on the above shortcomings.
Additional cleanup/doc originating from nhibernate#1788 but which should be here
| foreach (var resultSetHandler in multiSource.GetResultSetHandler()) | ||
| { | ||
| rowCount += resultSetHandler(reader); | ||
| reader.NextResult(); |
There was a problem hiding this comment.
I was trying to avoid code duplication in IQueryBatchItem implementations and from this point - reader.NextResult call should be responsibility of IQueryBatch implemention. And I've made suggestion that should allow to keep old interface/behavior intact.
But it seems your refactoring fixed one more issue - no async counterpart was generated for old GetResultSetHandler method.
So this implementation might be a better option as it's suppored by async generator.
There was a problem hiding this comment.
Well, about NextResult, I understand it could be considered the responsibility of the batcher. But at the same time, that is the queries who know how many result sets they have to consume. So there is no clear cut on that subject from my point of view.
| { | ||
| var future = | ||
| s | ||
| .CreateSQLQuery("select count(*) as count from EntitySimpleChild where Name like :pattern") |
There was a problem hiding this comment.
Firebird does not like as count part.
There was a problem hiding this comment.
Yeah, I should have thought using count as an alias is maybe not a good idea for all databases.
Fix SQL query for broader compatibility
|
From the release management perspective. I'm not sure if we need to display this issue in the release log. Thoughts? |
|
I agree. (But I am quite sure there is a bunch of similar cases, fixing changes before they were released.) So flagging as fix. |
Follow up to #1718. See its reviews for more details on the above shortcomings.