Skip to content

Conversation

mbellade
Copy link
Member

@mbellade mbellade commented Sep 8, 2025

Similar to #10851 but for query parameters. This also follows up addressing concerns with my recent change that lead to value-bind criteria params causing criteria query plan cache misses if not using the same instance.


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

@mbellade
Copy link
Member Author

mbellade commented Sep 8, 2025

While I was at it, since I had to re-instate LinkedIdentityHashMap, I tried implementing it in a way that we don't need wrappers or additional underlying collections to support it.

@beikov
Copy link
Member

beikov commented Sep 9, 2025

This also follows up addressing concerns with my recent change that lead to value-bind criteria params causing criteria query plan cache misses if not using the same instance.

I guess that changing this to use value equality is going to "somewhat" improve the situation, but I'm working on full caching right now on top of #10882 and would like to keep the equals/hashCode contracts of ValueBindJpaCriteriaParameter to be based on identity. Any concerns with that? Otherwise you'd also have to change QueryParameterBindingsImpl#parameterBindingMap to use this LinkedIdentityHashMap.

@mbellade
Copy link
Member Author

mbellade commented Sep 9, 2025

and would like to keep the equals/hashCode contracts of ValueBindJpaCriteriaParameter to be based on identity. Any concerns with that?

Well, the main driver for this change was to prevent trashing the query-plan cache any time a criteria query included a value-bind parameter, and going back to the value-based implementation was exactly meant to prevent that.

Otherwise you'd also have to change QueryParameterBindingsImpl#parameterBindingMap to use this LinkedIdentityHashMap

I see where you're coming from with this suggestion, but I doubt that would work: this would break the existing query bindings logic, as each time we would re-use a cached query plan (and maybe even a cached SQM interpretation) the query parameter used as key for the bindings would be a different instance than the ones cached from previous queries. I did try and confirmed it would be problematic, even e.g. in keyset paged queries when reusing named HQL query params that normally rely on their name for hash / equals.

We can switch back to the default (identity) equals / hash-code implementation for value-bind param when your work on the more powerful isCompatible method is used for query plan cache, but for now what we have works.

@beikov
Copy link
Member

beikov commented Sep 10, 2025

Yeah you're right. That implementation is probably the one that makes most sense in the meantime. I'll have to do the change I mentioned in my PR then.

@beikov
Copy link
Member

beikov commented Sep 10, 2025

But wouldn't it be better to use getJavaType().extractHashCode() and getJavaType().areEqual()?

@mbellade
Copy link
Member Author

That's what @theigl was suggesting on your pr, but that would require some additional work - we can play around with that as a further improvement to value-bind params wrt caching.

@beikov
Copy link
Member

beikov commented Sep 10, 2025

I did that now on this PR: #10899

@mbellade
Copy link
Member Author

Oh I see what you mean, sorry I though you were suggesting relying solely on the type of the param - we still rely on the value but use the correct java type helpers. Let me do that here as well.

@mbellade
Copy link
Member Author

Superseded by: #10902

@mbellade mbellade closed this Sep 10, 2025
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.

2 participants