Test added to your PR. It passes now. On Tue, Oct 16, 2018 at 10:02 AM Guillaume Smet <guillaume.s...@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 <st...@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.s...@gmail.com> >> wrote: >> >>> OK, ping me when you're ready! >>> >>> On Mon, Oct 15, 2018 at 1:39 PM Steve Ebersole <st...@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.s...@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.s...@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@lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>> >>>> _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
