Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

fix: adding property setter for table constraints, #1990 - #2092

Merged
Linchin merged 14 commits into
googleapis:mainfrom
lkhagvadorj-amp:feat/table-constraints-property-setter
Mar 4, 2025
Merged

fix: adding property setter for table constraints, #1990#2092
Linchin merged 14 commits into
googleapis:mainfrom
lkhagvadorj-amp:feat/table-constraints-property-setter

Conversation

@lkhagvadorj-amp

@lkhagvadorj-amplkhagvadorj-amp commented Dec 20, 2024

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes#1990 🦕


Overview

This pull request introduces enhancements to the google/cloud/bigquery/table.py file by adding setter methods and conversion functions. Additionally, a new unit test is added to ensure the functionality of the new setter method.

Enhancements to google/cloud/bigquery/table.py:

  • Added a setter method for table_constraints to allow setting primary key and foreign key information.
  • Added to_api_repr method to the ForeignKey class to return a dictionary representation of the object.
  • Added to_api_repr method to the TableConstraints class to return a dictionary representation of the object.

Unit tests:

  • Added a test for the table_constraints property setter to ensure it correctly updates the table properties.

@lkhagvadorj-amp
lkhagvadorj-amp requested review from a team and mrfaizalDecember 20, 2024 18:07
@google-cla

Copy link
Copy Markdown

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-labelproduct-auto-labelBot added the size: m Pull request size is medium. label Dec 20, 2024
@product-auto-labelproduct-auto-labelBot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Dec 20, 2024
@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 20, 2024
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 20, 2024
@Linchin

Copy link
Copy Markdown
Contributor

Thank you @lkhagvadorj-amp for making the contribution! The PR looks great, but could you do the following things so we can merge it:

  1. sign the CLA because it's part of the legal requirements
  2. add some unit tests: unit tests for both to_api_repr() methods, and more test cases for the setter, for 100% test coverage
  3. add a system test for this property

Thanks again and happy holidays!

@product-auto-labelproduct-auto-labelBot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 22, 2024
@lkhagvadorj-amp

Copy link
Copy Markdown
ContributorAuthor

@Linchin
Hi

Happy to contribute and thank you for the initial review!

I’ve pushed the testing changes—would be grateful if you could review them again.

Happy holidays!

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 30, 2024
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 30, 2024
@LinchinLinchin assigned Linchin and unassigned chalmerloweJan 2, 2025
Comment threadgoogle/cloud/bigquery/table.py Outdated
@Linchin

Copy link
Copy Markdown
Contributor

Thank you @lkhagvadorj-amp, I ran the test again, but it seems we need to fix several tests. I'm sorry that I have to trigger the presubmit tests manually, but you can run the system test locally by pytest tests/system/test_client.py, the lint test by nox -s lint, and mypy_samples test by nox -s mypy_samples. (You can install nox by pip install nox.)

@jmesterh

Copy link
Copy Markdown

Is this PR dead? Constraints are required for BigQuery CDC to function.

@lkhagvadorj-amp

lkhagvadorj-amp commented Feb 14, 2025

Copy link
Copy Markdown
ContributorAuthor

@jmesterh
Thank you for reminder.

I will work on this change on this weekend.
My apologies for late fixing. :)

@lkhagvadorj-amp
lkhagvadorj-ampforce-pushed the feat/table-constraints-property-setter branch from b646533 to 3dcbeeaCompareFebruary 21, 2025 11:38
@lkhagvadorj-amp
lkhagvadorj-amp requested a review from a team as a code ownerFebruary 21, 2025 11:38
@lkhagvadorj-amp
lkhagvadorj-ampforce-pushed the feat/table-constraints-property-setter branch 2 times, most recently from dd2c5ed to 62028bcCompareFebruary 21, 2025 11:49
@lkhagvadorj-amp

Copy link
Copy Markdown
ContributorAuthor

@Linchin
Hi , hope you're doing well! 👋

Could you please take another look at my PR when you get a chance?
Sorry about the many commits—I'll make sure to keep it cleaner next time. Thanks so much for your time! 🙏

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 24, 2025
@kokoro-teamkokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 24, 2025
Comment threadgoogle/cloud/bigquery/table.py Outdated
def __eq__(self, other):
if not isinstance(other, TableConstraints) and other is not None:
raise TypeError("The value provided is not a BigQuery TableConstraints.")
return (

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.

I think we need to add a test case for this case

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added more test on eq under unit test

@Linchin

Copy link
Copy Markdown
Contributor

Hi @lkhagvadorj-amp thanks for updating the PR, from the tests it seems we need some additional test coverage, and need to fix linting. Could you update the PR so we can pass these tests?

We are in the process of migrating our testing infra so there's a confusingly large number of tests that are failing, but for now the only ones that matter are "Kokoro" and "Kokoro linting-typing", and you can ignore the rest.

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 25, 2025
@yoshi-kokoroyoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 25, 2025
@Linchin

Copy link
Copy Markdown
Contributor

Thanks @lkhagvadorj-amp, there's still a test failing in linting, could you fix it too? Here's the message:

nox > mypy -p google --show-traceback
google/cloud/bigquery/table.py:3473: error: Incompatible types in assignment (expression has type "List[Dict[str, Any]]", target has type "Dict[str, List[str]]") [assignment]

@lkhagvadorj-amp

Copy link
Copy Markdown
ContributorAuthor

@Linchin
Hi thank you!
Fixed on linting error (mypy check).

Please let me know if you have any additional comments.

@LinchinLinchin added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 4, 2025
@kokoro-teamkokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Mar 4, 2025

@LinchinLinchin 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.

LGTM, thank you for your contribution!

@Linchin
Linchin merged commit f8572dd into googleapis:mainMar 4, 2025
@release-pleaserelease-pleaseBot mentioned this pull request Mar 4, 2025
@lkhagvadorj-amp

Copy link
Copy Markdown
ContributorAuthor

@Linchin
Yeaay!

My apologies for too many commits and looping again and again.

Happy to contribute!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.size: lPull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

table_constraints has no setter?

6 participants

@lkhagvadorj-amp@Linchin@jmesterh@chalmerlowe@kokoro-team@yoshi-kokoro