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

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 >

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

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