[
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
