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

Experimental support for the Decimal type #106

Closed
Tracked by #116
RobertCraigie opened this issue Nov 7, 2021 · 12 comments · Fixed by #338
Closed
Tracked by #116

Experimental support for the Decimal type #106

RobertCraigie opened this issue Nov 7, 2021 · 12 comments · Fixed by #338
Labels
kind/feature A request for a new feature. level/advanced priority/medium topic: types An issue or improvement to typing
Projects

Comments

@RobertCraigie
Copy link
Owner

RobertCraigie commented Nov 7, 2021

Why is this experimental?

Currently Prisma Client Python does not have access to the field metadata containing the precision of Decimal fields at the database level. This means that we cannot:

  • Raise an error if you attempt to pass a Decimal value with a greater precision than the database supports, leading to implicit truncation which may cause confusing errors
  • Set the precision level on the returned decimal.Decimal objects to match the database level, potentially leading to even more confusing errors.

To try and mitigate the effects of these errors you must be explicit that you understand that the support for the Decimal type is not up to the standard of the other types. You do this by setting the enable_experimental_decimal config flag, e.g.

generator py {
  provider                    = "prisma-client-py"
  enable_experimental_decimal = true
}
@RobertCraigie RobertCraigie added topic: types An issue or improvement to typing kind/feature A request for a new feature. labels Nov 7, 2021
@RobertCraigie RobertCraigie added this to To do in v1.0.0 via automation Nov 7, 2021
@RobertCraigie RobertCraigie changed the title Add support for the decimal type Add support for the Decimal type Nov 7, 2021
@RobertCraigie RobertCraigie added the process/candidate Candidate for the next release label Nov 13, 2021
@RobertCraigie RobertCraigie added this to the v0.3.1 milestone Nov 13, 2021
@RobertCraigie
Copy link
Owner Author

RobertCraigie commented Nov 14, 2021

This is more difficult to do right than I initially anticipated.

If we just naively serialise decimal.Decimal objects then there can be a loss of precision at the database level due to a mismatch between decimal.Decimals precision and the precision at the database level.

Due to how our query builder works we actually cannot warn or error when a decimal.Decimal with a precision greater than what the database supports is encountered as precision is configurable on a per-field basis.

I do not know the best solution for this issue at this point in time.

@RobertCraigie
Copy link
Owner Author

RobertCraigie commented Nov 14, 2021

Blocked by: prisma/prisma#10252

At this point in time I do not believe the workaround mentioned is a good enough solution.

I also do not want to introduce any footguns.

@RobertCraigie RobertCraigie removed the process/candidate Candidate for the next release label Nov 14, 2021
@RobertCraigie RobertCraigie removed this from the v0.3.1 milestone Nov 14, 2021
@RobertCraigie
Copy link
Owner Author

A solution I hadn't considered before for the aforementioned issue would be to simply process the data that prisma sends us and set the precision appropriate to that. However I do not know how decimal objects interact with each when the precision differs. This could also be confusing for consumers as the precision would be different for every object.

@danielweil
Copy link

How should we do currently if the Decimal type is not supported?

@RobertCraigie
Copy link
Owner Author

@danielweil you just can't use it unfortunately.

An alternative would be to use the String type at the database level and convert it to a decimal.Decimal yourself.

@tday
Copy link

tday commented Jan 31, 2022

Would you be willing to implement one of the workaround solutions while you work out the better long term fix? As is, I can't use this client at all since I cannot change the data type :(

@RobertCraigie
Copy link
Owner Author

@tday Considering that two people have asked for this feature I am willing to implement a workaround solution, however I am quite busy at the moment so while a complete solution might not be available for a little while I have made the necessary changes required for minimal support.

You can try it out by installing from the work in progress branch:

pip install -U git+https://github.com/RobertCraigie/prisma-client-py@wip/decimal

@tday
Copy link

tday commented Jan 31, 2022

@RobertCraigie Thanks for the quick turnaround! I think you need to add a quick line to the _get_sample_data function for the Decimal type as well.

Error: 
    sampled = self._get_sample_data()
  File "/Users/ayang/embark/genetic-predictor/beadchip/service/data-service/test/lib/python3.9/site-packages/prisma/generator/models.py", line 824, in _get_sample_data
    raise RuntimeError(f'Sample data not supported for {typ} yet')
RuntimeError: Sample data not supported for Decimal yet

@RobertCraigie
Copy link
Owner Author

@tday Sorry about that, I've pushed the fix.

@tday
Copy link

tday commented Feb 1, 2022

Thanks! Confirmed this works for Decimal on the WIP branch.

Given its sitting in a WIP feature branch, I was able to reduce my data type constraint down to double precision which maps to Float type in prisma/pydantic. This is cleaner in the meantime, but I'll keep an eye on this for the future.

@gakonst
Copy link

gakonst commented Mar 10, 2022

Hey folks - wonder how we can help push this forward? @RobertCraigie are there any outstanding items in your branch that need to get done? Happy to help.

@RobertCraigie
Copy link
Owner Author

I've spoken to the Prisma Team and while there is an open PR (prisma/prisma-engines#2660) to implement the internal feature required for us to be able to support Decimal properly they are wary about adding more data to the DMMF (essentially the Prisma AST) due to size concerns.

However, as this does seem to be a popular feature request I'll talk to them again and in the meantime I'm willing to implement a workaround until a proper solution is agreed upon.

In terms of what needs to be done before I can merge the in progress branch:

There may be other items but those are all that I can think of off the top of my head.

@gakonst Thanks for offering to help! Please be wary that adding support for a new type touches the deepest parts of this codebase, some of which are in need of a refactor and may be confusing for anyone new to the codebase. Reading through the contributing documentation will help although it is not extensive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for a new feature. level/advanced priority/medium topic: types An issue or improvement to typing
Projects
Development

Successfully merging a pull request may close this issue.

4 participants