openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sutter (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OPENJPA-2280) MappingTool ignores column precision / scale for some databases
Date Wed, 13 Feb 2013 18:26:13 GMT

    [ https://issues.apache.org/jira/browse/OPENJPA-2280?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13577803#comment-13577803
] 

Kevin Sutter commented on OPENJPA-2280:
---------------------------------------

Thanks for the proposed patch, Pinaki.  Here are my comments...

o  scripts/test.bat seems to be "extra"

o  Speaking of testing, don't we need some new or updated tests for these type of changes?
 Specifically, we could beef up TestTypesafeCriteria to be more stringent or explicit on the
expected outcomes.  In general though, we seem to be lacking in this "precision/scale" testing.

o  In DBDictionary.  Shouldn't we use the RoundingMode values instead of inventing new int
values?  More explicit and less guessing on where the values are coming from.  For example,
use RoundingMode.HALF_EVEN instead of setting roundingMode to 6.

o  I didn't notice any changes to AbstractDB2Dictionary to change the numericTypeName from
Double to Decimal.  Shouldn't that be part of this change?  If we go this route, then we'll
need documentation changes (at a minimum) for migration concerns.  Might require coding changes
as well to support both types.

o  The change set has many innocuous changes -- cosmetic changes that complicate the change
set, but provide no real value.  Personally, I would only make these type of changes when
I am changing code in that area.  The change set is pretty complicated as it is and these
"extra" changes just makes it more difficult to figure out what's important.

o  In the method setBigInteger, why check if val==null?  Couldn't you just pass val on through
to the setBigDecimal method and let it figure out what to do when val==null?

o  I'm not sure I fully understand your changes to Math.java.  Why are Const values special?
 

o  Same comment for AbstractExpressionBuilder.  Why is the check for expected!=null needed
now, but it wasn't needed in the past?  It looks like we processed the expected value in the
past when both o1 and o2 were not null, but not otherwise.

o  In Filters.promote(), it seems odd to do an explicit test for Number.class at the top of
this method.  What's the use case for this special condition?  Especially when this method
has lots of similar checks near the end of this method.  This type of conditional just seems
too "special case" and potentially fragile.

That's it.  Thanks again.
Kevin
                
> MappingTool ignores column precision / scale for some databases
> ---------------------------------------------------------------
>
>                 Key: OPENJPA-2280
>                 URL: https://issues.apache.org/jira/browse/OPENJPA-2280
>             Project: OpenJPA
>          Issue Type: Bug
>          Components: tooling
>    Affects Versions: 1.2.3, 2.3.0, 2.2.1
>            Reporter: Rick Curtis
>         Attachments: JIRA-2280.txt
>
>
> This JIRA is the same issue as reported by OPENJPA-1224.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message