Re: [hibernate-dev] CriteriaBuilder, aggregation and types

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(a)gmail.com&gt; 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-8fcc837a7... > . > > 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(a)gmail.com&gt; > 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... > > > > 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... > ). > > 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(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >