HHH-19745 Use custom isCompatible and cacheHashCode instead of equals/hashCode for SqmNode

[beikov]

[Please describe here what your change is about]

---

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

[gavinking]

I think it would be much better to solve this in the opposite way.

That is, I think we should keep using equals() / hashCode() to determine sameness of two ASTs from a "query interpretation" perspective.

And then when we have "special" node types (i.e. parameters) which have some additional different notion of equality, give those node types their own protocol.

Why do I say that? Well:

1. it's a change with much smaller impact: instead of changing 152 different node types, we change only those node types with their own special additional notion of equality, and
2. we retain the idea that "equality" of two ASTs maps to semantic equivalence of the queries. This is useful and nice!

[gavinking]

That is, I see the problem here as arising from the fact that certain special node types (that is, parameters) having their own special use of equals() which is distinct from semantic equivalence of the queries. It's those special node types which need a special protocol, not the whole AST.

[beikov]

Since the "entry-point" for the compatibility check for those "special node types" is at SqmExpression level (which is the top level type for most SQM AST nodes), this will roughly affect 90% of the code that is changed here anyway, so I don't see a good reason to rely on equals/hashCode.
Also, I added implementations of this "protocol" to a few classes that were previously missing implementations, which IMO just highlights how easy it is to forget adding equals/hashCode. It's much safer to rely on the Java compiler to tell us if a class misses an implementation for such vital methods.

IMO, identity of the SQM AST nodes is vital to understand if nodes really are the same rather than just equivalent for various cases (parameters, SqmFrom for locking, treat etc.). We can try and patch up all uses to express the requirement of identity equality, but so far, all downstream code was expecting equals/hashCode of SQM AST nodes is based on identity. With the changes to that assumption, we now received many bug reports, and some subtler ones may be coming still.

[gavinking]

Since the "entry-point" for the compatibility check for those "special node types" is at SqmExpression level (which is the top level type for most SQM AST nodes),

WDYM? I don't understand this. Apart from the query interpretation cache, exactly what code is it that is calling equals() on SQM nodes?

[beikov]

Since the "entry-point" for the compatibility check for those "special node types" is at SqmExpression level (which is the top level type for most SQM AST nodes),

WDYM? I don't understand this.

Most SQM nodes implement SqmExpression, just like the "special node types" JpaCriteriaParameter. So let's consider SqmComparisonPredicate#equals() as an example. Since the static type of leftHandExpression and rightHandExpression is SqmExpression, either could be a JpaCriteriaParameter, so we need to invoke the special protocol. Hence, we have to add methods to SqmExpression to allow invoking that. Given that most SQM nodes implement this interface, we end up having to change most of the files that I touched here anyway. So what's the point of one part of the AST rely on equals/hashCode and the other rely on a custom protocol? Why not use that custom protocol through the whole AST directly?

Apart from the query interpretation cache, exactly what code is it that is calling equals() on SQM nodes?

That's the thing, there are uses which we'd need to track down. SQM nodes appear in Set or as keys in Map, which will obviously use equals. Even the use of List#contains() will use equals.
I tracked down a few uses here before and @mbellade mentioned a few others to me the other day.

[gavinking]

Most SQM nodes implement SqmExpression, just like the "special node types" JpaCriteriaParameter. So let's consider SqmComparisonPredicate#equals() as an example. Since the static type of leftHandExpression and rightHandExpression is SqmExpression, either could be a JpaCriteriaParameter, so we need to invoke the special protocol.

I understand all that. What I'm saying is that parameter node types would have a definition of equals which is consistent with equivalence of the queries (from the point of view of the query interpretation cache, if you want to understand what I mean by "equivalence" here).

JpaCriteriaParameter should not have a special implementation of equals() which is inconsistent with this.

That's the thing, there are uses which we'd need to track down.

I think we should do that work, and track them down.

SQM nodes appear in Set or as keys in Map, which will obviously use equals.

It's my claim that they should probably not. Or, if they do, we need to understand very carefully why and whether that's correct.

What I'm saying is that it's in those cases, the cases where SQM nodes appear in Set or as keys in Mapthat we need a "special protocol" for equals().

[mbellade]

Adding to the discussion here, after my work with criteria parameters I did some experiments which confirmed that using identity-based sets and maps where we need to handle them works, even with the value-bind equals and hashCode implementations relying only on the value of the parameter for the purpose of query plan caching: #10890

I'm not suggesting the existing solution is better than the one proposed here, I just wanted to confirm that what we have can work, though there might be other bits and pieces that need adjustments like @beikov was saying.

[gavinking]

Awesome, that's great news!

[theigl]

Adding to the discussion here, after my work with criteria parameters I did some experiments which confirmed that using identity-based sets and maps where we need to handle them works, even with the value-bind equals and hashCode implementations relying only on the value of the parameter for the purpose of query plan caching: #10890

@mbellade: Sorry for chiming in, but can you also give your solution a try with the following equals/hashCode implementation for value-bind parameters:

 Objects.equals( getNodeType(), that.getNodeType() );

 Objects.hashCode( getNodeType() );

Relying on the value does not make much sense for the query plan cache, because it generates a separate entry for each distinct combination of values.

[mbellade]

Relying on the value does not make much sense for the query plan cache, because it generates a separate entry for each distinct combination of values.

Hey @theigl this is a valid improvement proposal, but I would stick with what we know worked before foro now - as you can see, we're still trying to figure out the best approach for criteria query plan caching, and it's very important to get that right, so let's keep the conversation here about that. You're free to open an improvement request on Jira, and perhaps give it a go yourself, we'd be happy to help once we have ironed this out.

[theigl]

Relying on the value does not make much sense for the query plan cache, because it generates a separate entry for each distinct combination of values.

Hey @theigl this is a valid improvement proposal, but I would stick with what we know worked before foro now - as you can see, we're still trying to figure out the best approach for criteria query plan caching, and it's very important to get that right, so let's keep the conversation here about that. You're free to open an improvement request on Jira, and perhaps give it a go yourself, we'd be happy to help once we have ironed this out.

There is https://hibernate.atlassian.net/browse/HHH-19556 created by Gavin, that I somewhat hijacked, but I can create another issue if necessary. My feeling is that only a solution that will eventually work with a type-based equality is a real solution to this issue. Value-based equality is not what users are expecting from the query plan cache and will probably do more harm than good when enabled in a larger application.

[gavinking]

My feeling is that only a solution that will eventually work with a type-based equality is a real solution to this issue.

As I've commented elsewhere, I doubt that comparing types is either safe, or necessary.

I'm skeptical that two queries can be different if they only differ in the type of a parameter. I mean, that parameter has to be compared or assigned to something, which has to have a matching type.

[beikov]

Superseded by #10899