[hibernate-dev] CriteriaBuilder, aggregation and types

[Guillaume Smet]

Hi,

We discussed a bit more with Fabio on Friday and we ended up discovering
that we have an issue with most Expressions that contain nested Expressions.

The Renderable contract is defined with these 3 methods:
    String render(RenderingContext renderingContext);

    default String renderProjection(RenderingContext renderingContext) {
        return render( renderingContext );
    }

    default String renderGroupBy(RenderingContext renderingContext) {
        return render( renderingContext );
    }

The issue is that most of the Expressions containting nested Expressions
only implement render().

This means that you basically do the following:
call renderProjection() -> expression1 only implement render() -> we call
render() on the nested expressions instead of renderProjection().

One possible workaround is to do what was done in SearchedCaseExpression
for all the Expressions containing nested expressions (e.g. implement the 3
methods and have a private method taking a lambda to propagate the
variation).

You can see an example of this change here:
https://github.com/hibernate/hibernate-orm/pull/2568/files#diff-8fcc837a76b1af31f11e16a01332fd1dR96
.

Note that it has to be done for *all* the Expressions containing nested
expressions. Either that or we simplify the Renderable contract to have
only one render() method and a parameter defining the context. That would
allow to avoid all these changes.

Thoughts?

-- 
Guillaume

On Thu, Oct 11, 2018 at 4:01 PM Guillaume Smet <guillaume.smet at gmail.com>
wrote:

> Hi,
>
> We have an interesting test case here
> https://hibernate.atlassian.net/browse/HHH-13001 about the use of
> LiteralHandlingMode with the criteria builder. It turns out that the
> problem is a bit more general than that.
>
> Let's take this (not so useful) example:
> query.multiselect(
>                         document.get( "id" ),
>                         cb.sum( cb.parameter( Long.class ) ) )
>                 .groupBy( document.get( "id" ) );
>
> It will lead to a NPE when trying to determine the return type of the sum
> in:
>
> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/dialect/function/StandardAnsiSqlAggregationFunctions.java#L157
>
> The fact is that we totally lose the type information along the way.
>
> I don't know if it's something addressed in 6 and if we should try to fix
> it in 5.4. In any case, I think having a workaround would be nice.
>
> There are a few paths we could follow:
> 1/ at least throw a more meaningful error and provide a workaround. I
> thought about forcing the use of the cast function but it doesn't work as
> we have an optimization not adding the cast function is the type is
> corresponding (see
> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/query/criteria/internal/expression/ExpressionImpl.java#L35).
> If we remove this one and provide a helpful error message, that could help.
> Note that I don't know if it's just an optimization or something mandated
> by the spec itself.
>
> 2/ we could try to add a cast automatically for these functions needing a
> proper type only when strictly necessary e.g. when you have a parameter (or
> a LiteralExpression transformed to a parameter). That would mean adding a
> method ExpressionImplementor (and change all our code to use
> ExpressionImplementor as it currently uses the JPA version mostly
> everywhere). That would be more transparent for the user but clearly a
> significant amount of (dumb) work/changes. And probably a nightmare to
> merge in 6 if this area has changed.
>
> Thoughts?
>
> --
> Guillaume
>