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