8000 feat: support load job option ColumnNameCharacterMap by Linchin · Pull Request #1952 · googleapis/python-bigquery · GitHub
[go: up one dir, main page]

Skip to content

feat: support load job option ColumnNameCharacterMap #1952

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

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

Linchin
Copy link
Contributor
@Linchin Linchin commented Jun 12, 2024

Fixes #1951 🦕

@Linchin Linchin requested review from a team as code owners June 12, 2024 22:34
@Linchin Linchin requested a review from obada-ab June 12, 2024 22:34
@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 Jun 12, 2024
Copy link
Collaborator
@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM.
I have one PREFERENCE to make the test code simpler and potentially easier to parse and maintain in the long run.

@@ -843,3 +843,42 @@ def test_parquet_options_setter_clearing(self):

config.parquet_options = None
self.assertNotIn("parquetOptions", config._properties["load"])

8000 def test_column_name_character_map_missing(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

PREFERENCE:

These tests are nearly identical. The only real change is the choice of setting and whether the outcome matches.

This feels like an ideal situation for a parameterized test.

I would recommend we shorten this code by about 27 lines by using a pytest parameterization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! It would be really nice if we could make the tests more compact. For the purpose of this PR, I just followed the same pattern as the rest of this test file. I think we can consider revamping the test as a whole so the style of the tests can be more consistent. I do wonder though, because we are not calling the exact same methods in each of the tests (sometimes we assign value in initialization, sometimes by calling the method, or directly accessing the _properties dict), is there a nice way to represent these in test parameterization?

@Linchin Linchin enabled auto-merge (squash) June 14, 2024 17:20
@Linchin Linchin merged commit 7e522ee into googleapis:main Jun 14, 2024
15 checks passed
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.

support load job option ColumnNameCharacterMap
2 participants
0