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

feat: add support for dataset.default_rounding_mode #1688

Merged
merged 2 commits into from
Oct 28, 2023

Conversation

Gaurang033
Copy link
Contributor

Fixes #1687

@Gaurang033 Gaurang033 requested review from a team as code owners October 18, 2023 18:34
@google-cla
Copy link

google-cla bot commented Oct 18, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/python-bigquery API. labels Oct 18, 2023
@Linchin Linchin requested review from chalmerlowe and Linchin and removed request for jainsahab October 19, 2023 18:45
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2023
Comment on lines 540 to 554
"""
optional[str]: it could have one of the following possible value.
ROUNDING_MODE_UNSPECIFIED: Unspecified will default to using ROUND_HALF_AWAY_FROM_ZERO.
ROUND_HALF_AWAY_FROM_ZERO: ROUND_HALF_AWAY_FROM_ZERO rounds half values away from zero when applying precision
and scale upon writing of NUMERIC and BIGNUMERIC values.
For Scale: 0 1.1, 1.2, 1.3, 1.4 => 1 1.5, 1.6, 1.7, 1.8, 1.9 => 2
ROUND_HALF_EVEN: ROUND_HALF_EVEN rounds half values to the nearest even value when applying precision and scale
upon writing of NUMERIC and BIGNUMERIC values.
For Scale: 0 1.1, 1.2, 1.3, 1.4 => 1 1.5 => 2 1.6, 1.7, 1.8, 1.9 => 2 2.5 => 2
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the docstring. I think its format is not passing the docs test, see testing logs for more detailed information. Here is an example for docstring format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. i fixed it and run nox -s docs without any error.

"""
return self._properties.get("defaultRoundingMode")

@default_rounding_mode.setter
Copy link
Contributor

Choose a reason for hiding this comment

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

The test coverage is not passing because the setter function is not covered. Could you please also add unit test for this? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Linchin should I modify this test_create_dataset_w_attrs ?

Copy link
Contributor

@Linchin Linchin Oct 20, 2023

Choose a reason for hiding this comment

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

I think it would be better to add several smaller tests (in parallel to test_create_dataset_with_default_rounding_mode() as you added), each for a case in our if conditions. These include:

  1. value is None
  2. value is not a string
  3. value not in possible_values
  4. value is in possible_values

Comment on lines 2300 to 2303
if kwargs.get("location"):
dataset.location = kwargs.get("location")
if kwargs.get("max_time_travel_hours"):
dataset.max_time_travel_hours = kwargs.get("max_time_travel_hours")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you have also opened #1683 that covers max_time_travel_hours. Let's make these changes over there, and confine this PR to only default_rounding_mode stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Linchin done. cherry pick the commit to that pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also delete these lines in this PR? Thank you!

@Linchin
Copy link
Contributor

Linchin commented Oct 19, 2023

@Gaurang033 Thank you for your contribution! The PR looks good except for a few comments that need to be addressed. Please let us know if you need any help with them :)


Raises:
ValueError: for invalid value types.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Linchin sorry. what change should i make ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Linchin done. made the changes. could you please review again and let me know if I need to make any other changes.

@Gaurang033 Gaurang033 force-pushed the feature/default_rounding_mode branch 3 times, most recently from 3c57278 to 5d43d87 Compare October 20, 2023 23:40
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 27, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 27, 2023
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 27, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 27, 2023
@Linchin
Copy link
Contributor

Linchin commented Oct 27, 2023

There's a mypy test error that's blocking this PR. Created #1705 to fix this.

@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2023
@Linchin Linchin enabled auto-merge (squash) October 28, 2023 06:04
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 28, 2023
@Linchin Linchin merged commit 83bc768 into googleapis:main Oct 28, 2023
16 of 17 checks passed
@Gaurang033 Gaurang033 deleted the feature/default_rounding_mode branch October 29, 2023 22:49
Linchin added a commit to Linchin/python-bigquery that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for default_rounding_mode at dataset creation
3 participants