FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: add decimal/numeric support by vi3k6i5 · Pull Request #620 · googleapis/python-spanner-django · GitHub

This repository was archived by the owner on Jun 8, 2026. It is now read-only.
/ python-spanner-django Public archive

feat: add decimal/numeric support - #620

Merged
vi3k6i5 merged 21 commits into
googleapis:masterfrom
vi3k6i5:decimal_numeric
May 19, 2021
ConversationCommits (21)ChecksFiles changed
Merged

feat: add decimal/numeric support#620
vi3k6i5 merged 21 commits into
googleapis:masterfrom
vi3k6i5:decimal_numeric

Conversation

vi3k6i5 commented May 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

larkee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the decimal_numeric branch 3 times, most recently from 38f030e to 3d014ae Compare May 10, 2021 13:44
vi3k6i5 requested a review from c24t May 10, 2021 13:47
vi3k6i5 changed the title feat: added decimal/numeric support feat: add decimal/numeric support May 10, 2021
c24t reviewed May 11, 2021
View reviewed changes

c24t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

c24t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 17, 2021

Copy link
Copy Markdown
Contributor Author

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 commented May 17, 2021

Copy link
Copy Markdown
Contributor Author

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 force-pushed the decimal_numeric branch from ec16a7d to 265955a Compare May 17, 2021 13:32
vi3k6i5 closed this May 18, 2021
vi3k6i5 reopened this May 18, 2021
vi3k6i5 closed this May 18, 2021
vi3k6i5 reopened this May 18, 2021
vi3k6i5 requested review from c24t and larkee May 18, 2021 13:47
vi3k6i5 closed this May 19, 2021
vi3k6i5 reopened this May 19, 2021

vi3k6i5 commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

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

c24t approved these changes May 19, 2021
View reviewed changes

c24t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread noxfile.py
Comment thread tests/unit/django_spanner/test_lookups.py Outdated
Comment thread django_spanner/operations.py
Comment thread django_spanner/features.py
vi3k6i5 mentioned this pull request Mar 2, 2026
Open
vi3k6i5 merged commit d09ad61 into googleapis:master May 19, 2021
This was referenced Apr 17, 2024
Closed
Closed
Closed
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

larkee Awaiting requested review from larkee
1 more reviewer
c24t c24t approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL