[hibernate-dev] CriteriaBuilder, aggregation and types

[Guillaume Smet]

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
>>
>