SQM cast target

[sebersole]

Re: https://hibernate.zulipchat.com/#narrow/stream/132094-hibernate-orm-dev/topic/6.2E0.20-.20cast.20target

In re-enabling some of the older HQL tests I ran into some failures due to expectations for cast targets. At the moment, 6.0 has a discrete, hard-coded set of allowable cast targets based on an IDENT whereas we have historically been a little more dynamic. As an example, the test where I ran into this was attempting ... cast( x as java.lang.String ) .... So that breaks because (1) it attempts to use a DOT-IDENT sequence and (2) because (even if we accepted DOT-IDENT) it is not one of the hardcoded targets.

Now granted, this is a trivial example and could easily be re-written as ... cast( x as string ) ..., ... cast( x as String ) ... etc. But I think we need to consider what set of targets we want to allow as valid. Part of that is backwards compatibility, but a large part of it is flexibility/usefulness.

As far as IDENT v. DOT-IDENT, we could also consider this like we do with the dmlTarget and entityName rules which are defined in terms of DOT-IDENT, but which efficiently return the String representation - the DOT-IDENT aspect becomes completely transparent.

I think the first thing to consider in terms of a solution is what we ultimately need to properly represent this target in the SQM / SQL trees.

In SQM, SqmCastTarget is just a node - all we really need to fully implement that contract is a JavaTypeDescriptor (well technically a SqmExpressable, but that only requires exposing JavaTypeDescriptor). Probably we'd ideally have the JdbcTypeDescriptor here too; its not really needed for this part, but might help in the SQM->SQL step; devil's in the details and I guess we'll see as we implement this. The thing I am trying to avoid here is requiring a full DomainType / AllowableFunctionReturnType. It will become apparent as we talk about the different ways to specify the cast-target.

In the SQL AST, it depends on how fully we want to implement the Expression contract (which CastTarget implements. If we want a full implementation, we need a MappingModelExpressable. Now the MappingModelExpressable contract itself is easy enough to implement here. Its really nothing more that a JdbcMapping and a bunch of "binding related" methods which we don't care about. At the moment, CastTarget is defined with a BasicValuedMapping. The "trickiest"part there is the need for a ManagedType, but JdbcMapping already happens to implement ManagedType so that should be fine.

I'd also suggest that we really want to resolve this early, maybe even as early as the SQM tree. The reasons being that (1) we have everything we need during SQM creation, and (2) we'd need some "type" info anyway to properly implement SqmExpressable#getExpressableJavaTypeDescriptor. If we consider ... cast( x as string ) what I mean here is that we need to know, at the very least, that "string" correlates to some form of JavaTypeDescriptor<String>. And to do that, we'd have to resolve that cast-target to something that gives us access to the JavaTypeDescriptor<String>. So what I suggest is:

class SqmCastTarget<T> extends AbstractSqmNode implements SqmTypedNode<T>, SqmVisitableNode {
    private JdbcMapping targetJdbcMapping;
    private final Long length;
    private final Integer precision;
    private final Integer scale;
    ...
    @Override
    public JavaTypeDescriptor<T> getExpressableJavaTypeDescriptor() {
        return targetJdbcMapping.getJavaTypeDescriptor();
    }
    ...

We'd then resolve the JdbcMapping during SQM building. The one hiccup I see with this approach is "type inference" which would be based on the SQM nodes. I'd have to play with that to see if it is a real problem. And to be honest, should "type inference" really effect the "type" of the cast-target? Not sure.

Then:

class CastTarget<T> implements Expression, SqlAstNode {
    private JdbcMapping targetJdbcMapping;
    private final Long length;
    private final Integer precision;
    private final Integer scale;
    ...
    @Override
    public JdbcMapping getExpressionType() {
        return targetJdbcMapping;
    }
    ...

As for what kind of things to accept as cast targets, I suggest we should at least discuss the following.

== BasicType

We already have this in 5.x. Basically any name that is registered in the BasicTypeRegistry. This already covers most (all?) of the hard-coded cases.

== JavaTypeDescriptor

This ties in with the new @JavaType / @JavaTypeRegistration approach to defining handling for basic-values model parts. The big questions here are:

1. Do we allow any and all known JavaTypeDescriptor entries in the JavaTypeDescriptorRegistry? Or do we allow JavaTypeDescriptor impls to optionally, additionally register themselves as cast targets?
2. Do we allow IDENT or DOT-IDENT? DOT-IDENT would be needed to support the "allow any and all" approach. If we decide to stick with IDENT, then we'd have to have some form of explicit registration under a "short name".

Functionally, this would rely on JavaTypeDescriptor#getRecommendedJdbcType to fill out the JdbcMapping.

== AttributeConverter

This one would almost certainly require changing to allow DOT-IDENT, unless we implement some form of optional registration for them.

For background, I bring this one up because it ties in nicely with another feature I want to implement Query parameter binding based on passing a converter as the type. This would really only be useful in cases where the type inference for the parameter is incorrect.

E.g. `query.setParameter( "p1", theValue, TheConverter.class );

Not saying I like this one, just for completeness.

== Others?

[sebersole]

#3912

[sebersole]

So I have a PR that implements the simple approach here. Is that enough? Or do y'all feel like the registry and registration aspect is important?

[beikov]

I don't think the registry is that important. If we have a real use case, we can still add it later.

[gavinking]

FTR we discussed this here:

https://hibernate.zulipchat.com/#narrow/stream/132094-hibernate-orm-dev/topic/6.2E0.20-.20cast.20target

[sebersole]

FWIW, the link is the very first line in the main discussion ;)

[gavinking]

Haha OK :-)

[sebersole]

I applied the branch upstream and will lock this as "resolved" per consensus

[gavinking]

@sebersole is this discussion dead? Can we close it?