Skip to content
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

Fix to "BigDecimal has mismatching scale value for given Decimal schema" #89

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

patelliandrea
Copy link

Fix to issue #44 as suggested in the comments

@ConfluentJenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@ghost
Copy link

ghost commented Jun 14, 2016

Hey @patelliandrea,
thank you for your Pull Request.

It looks like you haven't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@ewencp
Copy link
Contributor

ewencp commented Jun 14, 2016

ok to test

@@ -322,7 +323,12 @@ private static void convertFieldValue(ResultSet resultSet, int col, int colType,

case Types.NUMERIC:
case Types.DECIMAL: {
colValue = resultSet.getBigDecimal(col);
int scale = resultSet.getMetaData().getScale(col);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test this change in JdbcSourceTaskConversionTest where we test a bunch of other conversions?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we only need scale when the value is not NULL.
I suggest placing this in the corresponding else clause.

@patelliandrea
Copy link
Author

Hi, I've removed the double ;
The function passed all the tests in the JdbcSourceTaskConversionTest class

@patelliandrea
Copy link
Author

@ConfluentCLABot [clabot:check]

@ghost
Copy link

ghost commented Jun 15, 2016

@confluentinc It looks like @patelliandrea just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@ewencp
Copy link
Contributor

ewencp commented Jun 16, 2016

@patelliandrea I was curious about additional tests that actually trigger the need for this. I know we validate the scale in the logical type conversion which is why this is needed, but I'm not sure when it is occurring. setScale can throw an ArithmeticException if the scale is being reduced and the conversion would require rounding, and that isn't handled here. I would assume no database should be returning data with higher precision than requested, but since I don't know why/how this is happening in the first place I think it's probably better not to make an assumption like that.

@patelliandrea
Copy link
Author

@ewencp I've actually encountered the ArithmeticException because the conversion needed rounding so right now I'm using setScale with an hardcoded rounding mode, I'm working in order to make the rounding mode configurable.

if (bigDecimalValue == null)
colValue = null;
else
colValue = resultSet.getBigDecimal(col).setScale(scale);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to get the value twice? I believe you already have it in bigDecimalValue. Which is why I suggest:

colValue = bigDecimalValue.setScale(scale);

@cla-assistant
Copy link

cla-assistant bot commented Aug 1, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants