-
Notifications
You must be signed in to change notification settings - Fork 76
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 decimal precision (decimal.InvalidOperation decimal.DivisionImpossible error) #207
base: master
Are you sure you want to change the base?
Conversation
Why not making the default precision configurable in the target configuration? |
That would be a fine solution too. I can send an updated PR for this approach if you like.
This doesn't effect the database column types, only the in-python JSON Schema validation of the incoming data. But if the schema was updated with new precision then this code would recognize it. |
A PR for a config option would be great 👍 |
This reverts commit 6caf51c.
@hz-lschick done 👍 |
I seem to have a common problem with decimal.InvalidOperation: [<class 'decimal.DivisionImpossible'>] |
Hi @r-nyq we are looking into it and will try to merge it soon |
Problem
Error message (this is from target-snowflake but the problem is in the shared code):
The problem is that Singer's offical
tap-postgres
usesminimum
,maximum
, andmultipleOf
to effectively report the scale of the column.For example,
This breaks JSON schema validation as when validating
multipleOf
it tries to doDecimal('0.000913808181253534') % Decimal('1E-38')
, as the default decimal precision in python is too small (28 I believe).Solution
This is a well-known problem that has bitten several targets, for which there are several solutions. One is simply to set the precision to something arbitrarily high, like 40. Another, which the
pipelinewise-target-postgres
does, is to simply not allow precision higher than some threshold.Here, I ported a solution I wrote for
meltano/target-postgres
which sets Python's decimal precision to as large as it needs to be to match the schema. This solution was later ported tomeltano/target-snowflake
. Open to other solutions though. I would request that any solution also be ported todatamill-co/target-snowflake
.