Awesome, thanks! On Tue, Oct 16, 2018, 4:07 AM Guillaume Smet <guillaume.s...@gmail.com> wrote:
> 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
