Skip to content

feat: add decimal/numeric support#620

Merged
vi3k6i5 merged 21 commits intogoogleapis:masterfrom
vi3k6i5:decimal_numeric
May 19, 2021
Merged

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

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.

@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 c24t and larkee 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.

@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

@larkee larkee Awaiting requested review from larkee

1 more reviewer

@c24t 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

@vi3k6i5 @larkee @c24t

Comments