[hibernate-dev] CriteriaBuilder, aggregation and types

[Guillaume Smet]

Fabio put together a test in his PR:
https://github.com/hibernate/hibernate-orm/pull/2568/files#diff-ab60fb95866b43def421ef53163885ccR1

On Tue, Oct 16, 2018 at 4:38 AM Steve Ebersole <steve at hibernate.org> wrote:

> Are there actual tests for this besides the full zip on the issue?  How
> did you guys investigate this?
>
> On Mon, Oct 15, 2018 at 7:50 AM Guillaume Smet <guillaume.smet at gmail.com>
> wrote:
>
>> OK, ping me when you're ready!
>>
>> On Mon, Oct 15, 2018 at 1:39 PM Steve Ebersole <steve at hibernate.org>
>> wrote:
>>
>>> Let's discuss on Hip Chat in a few after I have woken up and had some
>>> coffee :)
>>>
>>>
>>> On Mon, Oct 15, 2018, 6:02 AM Guillaume Smet <guillaume.smet at gmail.com>
>>> wrote:
>>>
>>>> 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
>>>> >
>>>> _______________________________________________
>>>> hibernate-dev mailing list
>>>> hibernate-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>>
>>>