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.

Sign up for GitHub

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

Jump to bottom

feat: add decimal/numeric support #620

Merged
merged 21 commits into from
May 19, 2021
Merged

feat: add decimal/numeric support #620

merged 21 commits into from
May 19, 2021

Conversation

vi3k6i5
Copy link
Contributor

@vi3k6i5 vi3k6i5 commented May 9, 2021 •
edited
Loading

feat: added decimal/numeric support

Code changes involved:

  1. Currently decimal is supported by converting the value to float. Change this back to Decimal.
  2. Fix all related unit tests for supporting decimal
  3. Marking all tests that are not supported as skip and supported tests related to numeric/decimal enable them.
  4. Database validation needs to be removed.

fixes #617

vi3k6i5 added 8 commits April 6, 2021 14:05
@vi3k6i5
Merge pull request #1 from googleapis/master
924f1ee 
chore: add a Code of Conduct (googleapis#604)
@vi3k6i5
fix: lint_setup_py was failing in Kokoro is not fixed
df919be
@vi3k6i5
Merge pull request #2 from googleapis/master
ad5f15c 
fix: lint_setup_py was failing in Kokoro is now fixed (googleapis#607)
@vi3k6i5
Merge pull request #3 from googleapis/master
5ec9841 
fix: Remove un necessary file from code base (googleapis#608)
@vi3k6i5
Merge pull request #4 from googleapis/master
34c4dda 
feat: move migrations test modules to run against different emulator …
@vi3k6i5
Merge pull request #5 from googleapis/master
caf5a43 
feat: update workflow files to uniformly distribute the test modules …
@vi3k6i5
Merge pull request #6 from googleapis/master
129e41e 
feat: update docs and nox file to compile it (googleapis#610)
@vi3k6i5
Merge branch 'googleapis:master' into master
a1707fb
@vi3k6i5 vi3k6i5 requested a review from a team as a code owner May 9, 2021 07:01
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner-django API. label May 9, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 9, 2021
@vi3k6i5 vi3k6i5 force-pushed the decimal_numeric branch from d652bf2 to ceed7f0 Compare May 9, 2021 07:17
larkee
larkee reviewed May 10, 2021
View reviewed changes
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

Overall LGTM. My only concern is that the precision and scale of the Cloud Spanner NUMERIC should be clearly documented as I believe other databases support arbitrary precision NUMERIC.

@vi3k6i5 vi3k6i5 force-pushed the decimal_numeric branch 3 times, most recently from 38f030e to 3d014ae Compare May 10, 2021 13:44
@vi3k6i5 vi3k6i5 requested a review from c24t May 10, 2021 13:47
@vi3k6i5 vi3k6i5 changed the title feat: added decimal/numeric support feat: add decimal/numeric support May 10, 2021
c24t
c24t reviewed May 11, 2021
View reviewed changes
Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

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

Can we un-skip the django DecimalField tests now?

python django_tests/django/tests/runtests.py 'model_fields.test_decimalfield.DecimalFieldTests'

I see failures in test_fetch_from_db_without_float_rounding and test_fetch_from_db_without_float_rounding, probably worth checking if these are real errors here and only skipping the tests we need to.

Looks like we can revert #400 now too.

LGTM after getting the django tests to pass (or skipping if impossible) and splitting out the system tests.

tests/system/django_spanner/models.py Outdated Show resolved Hide resolved
django_spanner/operations.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@vi3k6i5 vi3k6i5 force-pushed the decimal_numeric branch 5 times, most recently from 61e63f7 to 489f1c2 Compare May 14, 2021 05:59
@vi3k6i5 vi3k6i5 force-pushed the decimal_numeric branch from 489f1c2 to 40a8fce Compare May 14, 2021 06:40
c24t
c24t reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

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

Do you need to update DatabaseValidation for the tests to run?

https://github.com/vi3k6i5/python-spanner-django/blob/40a8fcecb0dc1da321567b91524d8d0e9e2273e1/django_spanner/validation.py#L28

@vi3k6i5
Copy link
Contributor Author

vi3k6i5 commented May 17, 2021

Do you need to update DatabaseValidation for the tests to run?

https://github.com/vi3k6i5/python-spanner-django/blob/40a8fcecb0dc1da321567b91524d8d0e9e2273e1/django_spanner/validation.py#L28

removed validation for decimal field not supported by spanner.

@vi3k6i5
Copy link
Contributor Author

vi3k6i5 commented May 17, 2021

django_tests/django/tests/runtests.py 'model_fields.test_decimalfield.DecimalFieldTests'

most of these tests are currently running. The 2 skipped ones we cant add back because Spanner doesn't support them. Checked.

Reverted #400 .

@vi3k6i5 vi3k6i5 force-pushed the decimal_numeric branch from ec16a7d to 265955a Compare May 17, 2021 13:32
@vi3k6i5 vi3k6i5 closed this May 18, 2021
@vi3k6i5 vi3k6i5 reopened this May 18, 2021
@vi3k6i5 vi3k6i5 closed this May 18, 2021
@vi3k6i5 vi3k6i5 reopened this May 18, 2021
@vi3k6i5 vi3k6i5 requested review from larkee and c24t May 18, 2021 13:47
@vi3k6i5 vi3k6i5 closed this May 19, 2021
@vi3k6i5 vi3k6i5 reopened this May 19, 2021
@vi3k6i5
Copy link
Contributor Author

vi3k6i5 commented May 19, 2021

Workflows had issues yesterday Incident link so had to close and reopen the PR multiple times to retry all the actions.

c24t
c24t approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

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

A few non-blocking comments, feel free to merge it when you're ready.

noxfile.py Outdated Show resolved Hide resolved
tests/unit/django_spanner/test_lookups.py Outdated Show resolved Hide resolved
django_spanner/operations.py Show resolved Hide resolved
django_spanner/features.py Show resolved Hide resolved
@vi3k6i5 vi3k6i5 mentioned this pull request May 19, 2021
Open
@vi3k6i5 vi3k6i5 merged commit d09ad61 into googleapis:master May 19, 2021
This was referenced Apr 17, 2024
Closed
Closed
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c24t c24t c24t approved these changes

@larkee larkee Awaiting requested review from larkee

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for numeric field
3 participants
@vi3k6i5 @larkee @c24t