-
Notifications
You must be signed in to change notification settings - Fork 19
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
added decimal type #54
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 69.07% 69.37% +0.29%
==========================================
Files 3 3
Lines 152 160 +8
==========================================
+ Hits 105 111 +6
- Misses 47 49 +2
Continue to review full report at Codecov.
|
@@ -548,7 +571,7 @@ global const get_method_dict = Dict( | |||
# JDBC_COLTYPE_CLOB => 2005, | |||
# JDBC_COLTYPE_DATALINK => 70, | |||
JDBC_COLTYPE_DATE => getDate, | |||
JDBC_COLTYPE_DECIMAL => getFloat, | |||
JDBC_COLTYPE_DECIMAL => getBigDecimal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one of the reasons I hadn't implemented this yet is the fear that doing things this way means every numeric field is now Decimal object, which is hundreds of time slower. Even columsn with small or low precision numbers. Should I not worry about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also concerned about just what the meaning was of NUMERIC
. As far as I can tell, NUMERIC
really does mean decimal, and not float, see here. Even if this is true, it doesn't mean there aren't lots of places where people have carelessly declared fields that should be floats as NUMERIC
, but not much we can do about that I suppose.
Note that it might be worth considering using DecFP.jl instead of Decimals.jl, even though technically Decimals is the right package to use because it is arbitrary precision as BigDecimal claims to be. The reason is just that Decimal seems like it is probably ungodly slow. It even seemingly itroduces type instability for no reason ( Another concern is that I didn't find a good way to convert between Java's |
This fixes #53 by adding support for decimal types via Decimals.jl.