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
ISPN-14138 SQL Store fail to persist numbers with fractional portion #10308
Conversation
@gustavolira Do you think we can test this on the various DB providers? |
Hey @wburns and @gustavolira, does the test suite have tests that store and retrieve each kind of data type? Given this issue and the one with Booleans, it seems like it would be good to have! |
That is what I have expanded upon with the recent tests. To be honest my problem is I don't know what all the various types a user would use are :( But I can try to find others that we aren't testing, like I know float isn't explicitly tested, but should work. |
f5d1e98
to
eecba36
Compare
Updated to make sure Numeric with decimals work, added support for Real and Decimal types. |
@@ -110,6 +110,9 @@ int typeWeUse(int sqlType, String typeName) { | |||
} else if (typeName.toUpperCase().startsWith("BOOL")) { | |||
// Some databases store as int32 or something similar but have the typename as BOOLEAN or some derivation | |||
return Types.BOOLEAN; | |||
} else if (sqlType == Types.NUMERIC && scale == 0) { | |||
// If scale is 0 we don't want to use float or double types | |||
return Types.INTEGER; |
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.
Just to be on the safe side, should we use a BIGINT for this case or it is overkill?
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.
This keeps it inline with what we had before, as NUMERIC was always being assigned to an INTEGER (which is required for Oracle "boolean" types). The big difference is if the scale is not 0 we assign it to DOUBLE protostream type.
We could possibly assign it to BIGINT if the precision is higher though. I can try to prototype something like that. I had thought about it before but was trying to change as little as possible.
@wburns conflicts need to be resolved |
Yeah, I already resolved, was working on changing the types. But I am not having luck, I am going to say for this PR just leave it as is and we can enhance it further later. |
eecba36
to
03e4af6
Compare
Rebased. |
@wburns Couple of related failures in CI. |
Ah, yes, good catch.. Fixing. |
* Added support for Numeric * Added support for Decimal * Fixed Numeric to support decimals ** Numeric with 0 scale is still Integer
03e4af6
to
bd26131
Compare
Tests should be good now. |
Merged, thanks @wburns |
https://issues.redhat.com/browse/ISPN-14138