QueryHandler support to JPA SQL #457

Closed
TuomasKiviaho opened this Issue Jul 17, 2013 · 12 comments

Comments

Projects
None yet
2 participants
@TuomasKiviaho
Contributor

TuomasKiviaho commented Jul 17, 2013

QueryDSL SQL codegen has proper logic to avoid usage of BigDecimal when DECIMAL fits in range of Double. Problem is that MySQL driver and/or it's Hibernate Dialect counterpart is not that flexible and will return BigDecimal by default in these cases.

There already seems to be a TODO reservation for template based QueryHandler in AbstractJPASQLQuery. This surely isn't the right way fix this use case but my lack of knowledge lead me to check out this solution.

There might be a reason why QueryHandler hasn't been enabled by default for SQL queries but by patching (Abstract)JPASQLQuery to receive custom QueryHandler as optional constructor parameter gave me an opportunity to hack myself around the issue by using HibernateHandler enhanced with NumberConversions utility.

timowest added a commit that referenced this issue Jul 17, 2013

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jul 17, 2013

Member

I added QueryHandler to AbstractJPASQLQuery, but it doesn't fix your issue as you already noted. This commit though fixes it bdf0da8

Could you see if the latest SNAPSHOT fixes your issues?

Member

timowest commented Jul 17, 2013

I added QueryHandler to AbstractJPASQLQuery, but it doesn't fix your issue as you already noted. This commit though fixes it bdf0da8

Could you see if the latest SNAPSHOT fixes your issues?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jul 29, 2013

Member

Released in 3.2.2

Member

timowest commented Jul 29, 2013

Released in 3.2.2

@timowest timowest closed this Jul 29, 2013

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Aug 6, 2013

Contributor

Sorry about the delay and thanks for the fix which indeed works great. I noticed that there is no way to plug an arbitrary query handler to JPASQLQuery although it isn't crucial anymore.

My initial approach would have been to instruct Hibernate on how to map scalars (using SQLQuery.addScalar), but I couldn't find an universal mapping from normal java type (used by QueryDSL expression) to Hibernate's internal types and without deeper knowledge on how to properly navigate Expression hierarchy in order to retrieve path names I just gave up.

Contributor

TuomasKiviaho commented Aug 6, 2013

Sorry about the delay and thanks for the fix which indeed works great. I noticed that there is no way to plug an arbitrary query handler to JPASQLQuery although it isn't crucial anymore.

My initial approach would have been to instruct Hibernate on how to map scalars (using SQLQuery.addScalar), but I couldn't find an universal mapping from normal java type (used by QueryDSL expression) to Hibernate's internal types and without deeper knowledge on how to properly navigate Expression hierarchy in order to retrieve path names I just gave up.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 6, 2013

Member

Proper scalar mapping could be considered, if there is a signifact performance gain using them.

Member

timowest commented Aug 6, 2013

Proper scalar mapping could be considered, if there is a signifact performance gain using them.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 9, 2014

Contributor

I forgot completely to mention that I'm still having problems with this case when the problematic are is deeply nested.

I merely hacked Conversions class by using logic from other visitors. This implementation might be totally useless to you but here it is anyway https://gist.github.com/TuomasKiviaho/8333112. I think that it would be beneficial to have some sort of AbstractVisitor (as seen in the gist) which would complexities of the traversal. Now this logic seems to be scattered to several visitors and even FactoryExpressionUtils has it's own way. It's quite easy to for instance forget to give ProjectionRole a separate treatment.

Contributor

TuomasKiviaho commented Jan 9, 2014

I forgot completely to mention that I'm still having problems with this case when the problematic are is deeply nested.

I merely hacked Conversions class by using logic from other visitors. This implementation might be totally useless to you but here it is anyway https://gist.github.com/TuomasKiviaho/8333112. I think that it would be beneficial to have some sort of AbstractVisitor (as seen in the gist) which would complexities of the traversal. Now this logic seems to be scattered to several visitors and even FactoryExpressionUtils has it's own way. It's quite easy to for instance forget to give ProjectionRole a separate treatment.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 10, 2014

Member

@TuomasKiviaho could you give me an example of a case that still fails? Could you create a separate ticket for the expression modification?

Member

timowest commented Jan 10, 2014

@TuomasKiviaho could you give me an example of a case that still fails? Could you create a separate ticket for the expression modification?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 11, 2014

Contributor

While updating to the latest release, I noticed that the gist had stopped working. I think it has something to do with the added Enum support so I just removed the hack altogether. I see that decimal ranges have also been fine-tuned so with any luck that might never happen.

Contributor

TuomasKiviaho commented Jan 11, 2014

While updating to the latest release, I noticed that the gist had stopped working. I think it has something to do with the added Enum support so I just removed the hack altogether. I see that decimal ranges have also been fine-tuned so with any luck that might never happen.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 11, 2014

Contributor

Correction, gist seems to be fine (but missing new enum support). Problem was within pre-generated sql classes. By upgrading the plugin, I got them working again.

Contributor

TuomasKiviaho commented Jan 11, 2014

Correction, gist seems to be fine (but missing new enum support). Problem was within pre-generated sql classes. By upgrading the plugin, I got them working again.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 13, 2014

Contributor

Ok, I've figured out the scenario. Expression coming to native query transformation is of form new Tuple(bean1, bean2) meaning that in this case the problematic expression is buried under an extra FactoryExpression layer which is not expanded. I suspect that using FactoryExpressionUtils (to deprecate NumberConversions) might solve this case.

Contributor

TuomasKiviaho commented Jan 13, 2014

Ok, I've figured out the scenario. Expression coming to native query transformation is of form new Tuple(bean1, bean2) meaning that in this case the problematic expression is buried under an extra FactoryExpression layer which is not expanded. I suspect that using FactoryExpressionUtils (to deprecate NumberConversions) might solve this case.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 13, 2014

Contributor

Cross linking to Issue #626 and #623. I try to fix my particular case with maven plugin's numeric mappings configuration

Contributor

TuomasKiviaho commented Jan 13, 2014

Cross linking to Issue #626 and #623. I try to fix my particular case with maven plugin's numeric mappings configuration

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 16, 2014

Contributor

I was wondering the original request could be enabled, because it wasn't explicitly declined either. Namely if AbstractJPASQLQuery's queryHandler could be initialized through a delegate just as with AbstractJPAQuery. May be using a new JPASQLTemplates that has SQLTemplates and JPATemplates as mixins. This would enable user based customization such as scalar mappings etc when dealing with hibernate for instance

Contributor

TuomasKiviaho commented Jan 16, 2014

I was wondering the original request could be enabled, because it wasn't explicitly declined either. Namely if AbstractJPASQLQuery's queryHandler could be initialized through a delegate just as with AbstractJPAQuery. May be using a new JPASQLTemplates that has SQLTemplates and JPATemplates as mixins. This would enable user based customization such as scalar mappings etc when dealing with hibernate for instance

timowest added a commit that referenced this issue Jan 16, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 16, 2014

Member

I added now some additional constructors. Sorry that this was missing in the original fix. I might have misinterpreted your request.

Member

timowest commented Jan 16, 2014

I added now some additional constructors. Sorry that this was missing in the original fix. I might have misinterpreted your request.

@timowest timowest added this to the 3.2.2 milestone Apr 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment