New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default mapping for numeric types are incorrect #1194

Closed
robertandrewbain opened this Issue Feb 10, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@robertandrewbain
Member

robertandrewbain commented Feb 10, 2015

NUMERIC and DECIMAL should map to BigDecimal by default, rather than the current mapping of

17-...,? -> BigDecimal
0-16,? -> Double

which causes non-floating pointing numbers to be mapped to floating points number, which should never be used to represent exact values. See
http://stackoverflow.com/questions/28371179/floating-point-types-returned-in-orm-dsl, http://docs.oracle.com/cd/B19306_01/java.102/b14188/datamap.htm, http://www.cs.mun.ca/~michael/java/jdk1.1.5-docs/guide/jdbc/getstart/mapping.doc.html

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 10, 2015

Member

I agree that this would be valid change and could be applied to the default mappings for numeric types with fractional digits.

@Shredder121 What do you think?

Member

timowest commented Feb 10, 2015

I agree that this would be valid change and could be applied to the default mappings for numeric types with fractional digits.

@Shredder121 What do you think?

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Feb 10, 2015

Member

I also think it is a valid change.
As seen in the second link, NUMERIC and DECIMAL should be mapped to BigDecimal.

Member

Shredder121 commented Feb 10, 2015

I also think it is a valid change.
As seen in the second link, NUMERIC and DECIMAL should be mapped to BigDecimal.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 11, 2015

Member

Ok. I'd still check for the decimal digits though, so the change would be only to get rid of the Double mapping.

@robertandrewbain @Shredder121 Is that ok with you?

Member

timowest commented Feb 11, 2015

Ok. I'd still check for the decimal digits though, so the change would be only to get rid of the Double mapping.

@robertandrewbain @Shredder121 Is that ok with you?

@robertandrewbain

This comment has been minimized.

Show comment
Hide comment
@robertandrewbain

robertandrewbain Feb 11, 2015

Member

@timowest yes, that sounds good.

Member

robertandrewbain commented Feb 11, 2015

@timowest yes, that sounds good.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Feb 11, 2015

Member

Yes that sounds good indeed.

Member

Shredder121 commented Feb 11, 2015

Yes that sounds good indeed.

@timowest timowest added this to the 4.0.0 milestone Feb 11, 2015

@timowest timowest added the progress label Feb 12, 2015

@Shredder121 Shredder121 closed this in #1200 Feb 17, 2015

@borislubimov

This comment has been minimized.

Show comment
Hide comment
@borislubimov

borislubimov Feb 18, 2015

Hi! In querydsl version 3.2.4 oracle raw NUMBER datatype (without precision and scale) was mapped to Double, but since version 3.3.0 it maps to BigInteger. Current behavior is incorrect because NUMBER datatype stores fixed and floating-point numbers (http://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1832)
Could you tell, please, if this behavior will be fixed after this update?

borislubimov commented Feb 18, 2015

Hi! In querydsl version 3.2.4 oracle raw NUMBER datatype (without precision and scale) was mapped to Double, but since version 3.3.0 it maps to BigInteger. Current behavior is incorrect because NUMBER datatype stores fixed and floating-point numbers (http://docs.oracle.com/cd/B28359_01/server.111/b28318/datatype.htm#CNCPT1832)
Could you tell, please, if this behavior will be fixed after this update?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 18, 2015

Member

@borislubimov The behaviour will be fixed in the 3.6.2 and 4.0.0 releases. 3.6.2 will be out at the end of month.

Member

timowest commented Feb 18, 2015

@borislubimov The behaviour will be fixed in the 3.6.2 and 4.0.0 releases. 3.6.2 will be out at the end of month.

@borislubimov

This comment has been minimized.

Show comment
Hide comment
@borislubimov

borislubimov Feb 18, 2015

Thank you for quick reply!

borislubimov commented Feb 18, 2015

Thank you for quick reply!

@robertandrewbain

This comment has been minimized.

Show comment
Hide comment
@robertandrewbain

robertandrewbain Mar 9, 2015

Member

@timowest I've just used querydsl-maven-plugin version 3.6.2 (all querydsl packages are at 3.6.2) to generate-sources against an Oracle database. A NUMBER(13,6) is still mapping to a Double. I believed that the changes made to resolve this bug would have meant that a NUMBER(13,6) would map to a BigDecimal. Am I missing anything obvious?

Member

robertandrewbain commented Mar 9, 2015

@timowest I've just used querydsl-maven-plugin version 3.6.2 (all querydsl packages are at 3.6.2) to generate-sources against an Oracle database. A NUMBER(13,6) is still mapping to a Double. I believed that the changes made to resolve this bug would have meant that a NUMBER(13,6) would map to a BigDecimal. Am I missing anything obvious?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 9, 2015

Member

Sorry, the notice above was incorrect, there was only a PR for the 4.* line. Since this change breaks backwards compatibility and the mapping can be easily overriden it was only included in the 4 line.

Member

timowest commented Mar 9, 2015

Sorry, the notice above was incorrect, there was only a PR for the 4.* line. Since this change breaks backwards compatibility and the mapping can be easily overriden it was only included in the 4 line.

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