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

Fabio put together a test in his PR: https://github.com/hibernate/hibernate-orm/pull/2568/files#diff-ab60fb958... On Tue, Oct 16, 2018 at 4:38 AM Steve Ebersole <steve(a)hibernate.org&gt; 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(a)gmail.com&gt; > wrote: > >> OK, ping me when you're ready! >> >> On Mon, Oct 15, 2018 at 1:39 PM Steve Ebersole <steve(a)hibernate.org&gt; >> 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(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 >>>> >>>