HHH-18855: Subselect fetches not always generated in the same situation #9269
Conversation
leonschenk changed the title leonschenk force-pushed the HHH-18855 branch from 93395cb to 3f6ac42 Compare 
| Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
leonschenk force-pushed the HHH-18855 branch from 43c3737 to cda8ba1 Compare leonschenk force-pushed the HHH-18855 branch from cda8ba1 to 1d7b815 Compare 
| this.restrictivePart = restrictivePart; | ||
| this.lockOptions = lockOptions.makeCopy(); | ||
| this.jdbcParameters = jdbcParameters; | ||
| this.sqlAst = sqlAst; |
There was a problem hiding this comment.
Storing the whole SQL AST tree here means having to keep a complex structure around for every single-id load plan, which means for every mapped entity type. It would be best to avoid this, especially seems the SubselectFetch#createRegistrationHandler method apparently doesn't use it anymore apart from a specific case.
There was a problem hiding this comment.
I see what you mean here. Each SingleIdLoadPlan is stored in some sort of cache, it doesn't make much sense to keep around the queryast as well.
| final SubselectFetch.RegistrationHandler subselectRegistrationHandler = SubselectFetch.createRegistrationHandler( | ||
| session.getPersistenceContextInternal().getBatchFetchQueue(), | ||
| sqlAst, | ||
| jdbcParameters, | ||
| jdbcParameterBindings | ||
| ); |
There was a problem hiding this comment.
It would be great to avoid having to pass the sqlAst here. If you look at the SubselectFetch.createRegistrationHandler implementation, you can see it's used for 2 things:
- checking whether the query has at least 1 root
- passing the first
TableGroupto theStandardRegistrationHandlerconstructor
Regarding point 2., the constructor parameter is never actually used - so we can deprecate this constructor and create one without the TableGroup parameter.
As for point 1., in SingleIdLoadPlan we always know the generated query has at least one root (we can be extra sure by adding an assert !sqlAst.getQuerySpec().getFromClause().getRoots().isEmpty() in the constructor) - so we don't really need to pass it for this check. I suggest creating a third signature of the createRegistrationHandler with the same parameters but no TableGroup, and deprecate the one with it.
There was a problem hiding this comment.
@mbladel You might have overlooked something, because as far as I can see the QuerySpec (from sqlAst) is used for the generation of the SubselectFetch query. The original query is copied into the subselect query 'in (... original query ... )'.
It might be possible to generate de subselection query by other means (ie recreate the original queryspec while subselecting)? Otherwise I do not think we can do without at least the original queryspec. What is your opinion on this?
There was a problem hiding this comment.
You're right, and e.g. EntityBatchLoaderArrayParam already stores the SQL AST to use in subselect registration handlers, so we might do the same here.
| @Jpa(annotatedClasses = { | ||
| GenerateSubselectsOnJoinedAssociationTest.LazySelectRoot.class, | ||
| GenerateSubselectsOnJoinedAssociationTest.LazySelectNode.class | ||
| }) |
There was a problem hiding this comment.
Small note, but if you want you can use @DomainModel instead of @Jpa, together with the @SessionFactory annotation they will allow your test methods to accept a SessionFactoryScope so you won't need to call unwrap.
| for ( final LazySelectNode node : root.nodes ) { | ||
| assertThat( Hibernate.isInitialized( ( (LazySelectNode) Hibernate.unproxy( node ) ).children ), | ||
| is( false ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
It would be nice to also test if the registered subselect fetches actually work when trying to initialize the children collections, and make some assertions on their contents as well.
| @mbladel Thank you for your review. I will do the required work on the points that you mentioned. |
| @leonschenk could you please also test if |
There is indeed a problem here. I used the following setup to enable lazy vs eager fetching: However, the sqlAst is generated at setup-time where no fetchprofile was active. This means lazy fetchtype was selected, bacause it is the default for manyToMany. The 'cached' sqlAst is used because isLoadPlanReusable returns true (fetchprofile is not registered on the EntityPersister, but it should be because it affects the loadplan actually). Though the described problem has not much to do with the original problem, and may be a succesive ticket. |
| I believe the "problem" you mention is not really an issue, it's more of an improvement: if Could you please try just mapping the association as |
2 participants
I have come to the conclusion that this must be a bug, because the same query is executed but the effect is different. Here is a testcase that will succeed when fixed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-18855