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

AVRO-3521: "Scale" property from decimal object #1689

Merged
merged 4 commits into from
Jun 17, 2022
Merged

AVRO-3521: "Scale" property from decimal object #1689

merged 4 commits into from
Jun 17, 2022

Conversation

izveigor
Copy link
Contributor

@izveigor izveigor commented May 17, 2022 •
edited
Loading

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

RyanSkraba
RyanSkraba requested changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

This LGTM, can you just fix the error messages to reflect the corrected behaviour! This should be included in the next minor release.

lang/py/avro/io.py Outdated
@@ -699,7 +699,7 @@ def read_data(self, writers_schema: avro.schema.Schema, readers_schema: avro.sch
warnings.warn(avro.errors.IgnoredLogicalType(f"Invalid decimal precision {precision}. Must be a positive integer."))
return decoder.read_bytes()
scale = writers_schema.get_prop("scale")
if not (isinstance(scale, int) and scale > 0):
if not (isinstance(scale, int) and scale >= 0):
warnings.warn(avro.errors.IgnoredLogicalType(f"Invalid decimal scale {scale}. Must be a positive integer."))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
warnings.warn(avro.errors.IgnoredLogicalType(f"Invalid decimal scale {scale}. Must be a positive integer."))
warnings.warn(avro.errors.IgnoredLogicalType(f"Invalid decimal scale {scale}. Must be a non-negative integer."))

@izveigor
Copy link
Contributor Author

Hello, RyanScraba!
Thanks for advice. I changed all files.

RyanSkraba
RyanSkraba approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution.

@RyanSkraba RyanSkraba merged commit 659917b into apache:master Jun 17, 2022
RyanSkraba added a commit that referenced this pull request Jun 17, 2022
@izveigor @RyanSkraba
AVRO-3521: "Scale" property from decimal object (#1689)
b5bd312 
* AVRO-3521

* Fixed style for "AVRO-3521"

* Changed from 'a positive integer' to 'a non-negative integer' (by RyanSkraba's improvement).

* Update error message in test

Co-authored-by: Ryan Skraba <ryan@skraba.com>
czf pushed a commit to czf/avro that referenced this pull request Jun 21, 2022
@izveigor @RyanSkraba @czf
AVRO-3521: "Scale" property from decimal object (apache#1689)
5ecf3b6 
* AVRO-3521

* Fixed style for "AVRO-3521"

* Changed from 'a positive integer' to 'a non-negative integer' (by RyanSkraba's improvement).

* Update error message in test

Co-authored-by: Ryan Skraba <ryan@skraba.com>
@izveigor izveigor deleted the scale branch June 23, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanSkraba RyanSkraba RyanSkraba approved these changes

Assignees
No one assigned
Labels
Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@izveigor @RyanSkraba