HHH-19632 Give access to optional parameter info in SqlAstTranslator

[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-19632
https://hibernate.atlassian.net/browse/HHH-16283

[sebersole]

Preliminary thoughts for discussion. I need to work on something else, so wanted to get this back to you asap.

[sebersole]

Seems to me we ought to be deprecating the older forms here. I've noticed this pattern a bit recently where the number of overloads for newish, often incubating, methods continues to grow. Which makes refactoring more difficult than need be.

Here, e.g.

@Deprecated
default SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
        SessionFactoryImplementor sessionFactory, 
        SelectStatement statement) {
    return buildSelectTranslator( sessionFactory, statement, null );
}

SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
        SessionFactoryImplementor sessionFactory, 
        SelectStatement statement, 
        @Nullable SqlParameterInfo parameterInfo);

Or, since this is an SPI[1] and we probably want to make it easier for implementors, maybe even flip that:

@Deprecated
SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
        SessionFactoryImplementor sessionFactory, 
        SelectStatement statement);

SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
        SessionFactoryImplementor sessionFactory, 
        SelectStatement statement, 
        @Nullable SqlParameterInfo parameterInfo) {
    return buildSelectTranslator( sessionFactory, statement );
}

[1] This whole org.hibernate.sql.ast package is incubating and we really should come back and apply proper api/spi splits since they don't always reflect reality atm.

[beikov]

The buildSelectTranslator and buildMutationTranslator variants that don't pass SqlParameterInfo are actually still used, to avoid adding , null to a bunch of call sites where we build the AST manually and hence don't have that SqlParameterInfo available. Since SqlParameterInfo is only used to de-duplicate JdbcParameter, passing null SqlParameterInfo is fine generally.

Making these methods default would make sense though.

default SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
        SessionFactoryImplementor sessionFactory, 
        SelectStatement statement) {
    return buildSelectTranslator( sessionFactory, statement, null );
}
SqlAstTranslator<? extends JdbcOperationQueryMutation> buildMutationTranslator(
        SessionFactoryImplementor sessionFactory, 
        MutationStatement statement) {
    return buildMutationTranslator( sessionFactory, statement, null );
}

Let me know what you want me to do with this.

[sebersole]

I know that they are still used. My point though, iiuc, was that the parameter is already expected to accept nulls. So why keep around a form that is redundant?

[beikov]

To allow callers pass fewer arguments. It's just convenience.

[sebersole]

Here is the counter-argument which is fresh in my mind from my work to refactor JdbcOperation - https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/sql/exec/spi/JdbcSelectExecutor.java

At the very least, we can make these default simply passing null. Every single target already has @Nullable for that parameter.

But at some point we need to stop growing these overloads because it makes refactoring challenging.

[sebersole]

I'll look through the code more, but first thought was - is parameterIdCount ever != parameterIdMap.size()?

[beikov]

Yes, if you use e.g. from MyEntity e where e.id = :id and e.id = :id, then the parameterIdCount == 1 whereas parameterIdMap.size() == 2

[sebersole]

To make sure I understand... You get 2 JdbcParameter refs here, but both map to the same parameterId?

[beikov]

Correct, because they both originate from the same SqmParameter.

[sebersole]

And we don't have the ability to simply assign this value to JdbcParameter when we create it?

[beikov]

Theoretically, we could, but that would mean at construction time of a JdbcParameter, we'd have to know the "construction context", which works for SQM translated queries, but maybe not that well for manually created queries. Using null parameter ids for such non-SQM translated queries to mean "this parameter is unique" is a possibility though. The only thing we'd be missing then is the knowledge about the amount of parameter ids, but that really is only there to be able to pre-size collections.

So yeah, we can definitely do it that way as well. Do you want me to change the code?

[sebersole]

I think its cleaner than having to track it separately and ask that separate contract for the information.
But it really comes back to whether it is feasible.

As for pre-sizing collections, that's definitely a nice-to-have. Though I do wonder how important it is in reality since there are minimum sizes for most collection impls and I'm just not sure how frequently we go over that. INSERT and UPDATE for sure will likely go over due to passing values, but DELETE and SELECT generally would not.

[beikov]

Superseded by #10672