[hibernate-dev] CriteriaBuilder, aggregation and types

[Steve Ebersole]

Awesome, thanks!

On Tue, Oct 16, 2018, 4:07 AM Guillaume Smet <guillaume.smet at gmail.com>
wrote:

> Test added to your PR. It passes now.
>
> On Tue, Oct 16, 2018 at 10:02 AM Guillaume Smet <guillaume.smet at gmail.com>
> wrote:
>
>> 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
>>>>>>
>>>>>