Skip to content

Conversation

@jrenaat
Copy link
Member

@jrenaat jrenaat commented Feb 20, 2025


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-19115

return handleResults( naturalIds, session, lockOptions );
}

protected <K> List<E> handleResults( K[] naturalIds, SharedSessionContractImplementor session, LockOptions lockOptions ) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'lockOptions' is never used.
@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch from 6fd66cc to 0100465 Compare February 21, 2025 20:10
@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch from 0100465 to 2d3c30f Compare March 12, 2025 21:46
@jrenaat jrenaat changed the title HHH-19115 - implement ordered multiloading with natural ids - WIP HHH-19115 - implement ordered multiloading with natural ids Mar 12, 2025
@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch 2 times, most recently from 176dbff to 1092ed5 Compare March 13, 2025 21:18
@jrenaat jrenaat requested a review from beikov March 13, 2025 22:10
}

@Test
@SkipForDialectGroup(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skip blocks here and below are a bit confusing IMO as there is no javadoc comment on why the skips are necessary. Also, I'm a bit surprised that a multi-load of 3 composite natural ids only requires 3 parameters. What is rendered SQL for this? I would expect this always requires 6 parameters i.e. where (ssn,ssn2) in ( (?,?), (?,?), (?,?) ) because tuples can't be passed as a single parameter AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex that i'm using for the tests that exclude Sybase, SQLServer, DB2 and hsqldb are looking at the (?,?) pattern, ergo, it checks whether there are 3
Sybase, SQLServer, DB2 and hsqldb produce different sql queries that effectively contain 6 '?', but using MultiKeyLoadHelper.supportsSqlArrayType for this didn't do the trick
I'll add a comment to the tests to clarify this.

@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch 2 times, most recently from 545a1ad to 0ee5465 Compare March 14, 2025 17:01
@jrenaat
Copy link
Member Author

jrenaat commented Mar 14, 2025

@beikov Wrt to our conversation in zulip, i checked whether using a @DialectFeatureCheck as you suggested would be viable, but IMHO it isn't because I don't have acccess for now to the element that differentiates the dialects; it would be much uglier to hack something than to just leave the test as it was conceived.
Again, I want to have a clear assert for those tests that support row value syntax in IN lists that the generated sql effectively contains '(?,?)' for each compound natural id, I think it is important.

@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch 3 times, most recently from 2030b2e to b5b62ec Compare March 14, 2025 17:25
@jrenaat jrenaat changed the title HHH-19115 - implement ordered multiloading with natural ids HHH-19115 - implement ordered multiloading with natural ids - DONOTMERGE Mar 20, 2025
(also corrected a couple of typo's I came across)

Signed-off-by: Jan Schatteman <[email protected]>
@jrenaat jrenaat force-pushed the HHH-19115_orderedMultiNaturalIdLoad branch from b5b62ec to 14b5e9a Compare March 20, 2025 17:12
@jrenaat jrenaat changed the title HHH-19115 - implement ordered multiloading with natural ids - DONOTMERGE HHH-19115 - implement ordered multiloading with natural ids Mar 20, 2025
@jrenaat jrenaat merged commit ef22641 into hibernate:main Mar 20, 2025
23 of 26 checks passed
@jrenaat jrenaat deleted the HHH-19115_orderedMultiNaturalIdLoad branch March 20, 2025 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants