Strikerrx01 commented on code in PR #34135:
URL: https://github.com/apache/beam/pull/34135#discussion_r1979151349
##########
sdks/python/apache_beam/io/gcp/bigquery_tools.py:
##########
@@ -1763,7 +1819,7 @@ def get_beam_typehints_from_tableschema(schema):
schema = get_bq_tableschema(schema)
typehints = []
for field in schema.fields:
- name, field_type, mode = field.name, field.type.upper(), field.mode.upper()
+ name, field_type, mode = field.name, field_type.upper(), field.mode.upper()
Review Comment:
@stankiewicz You're right, this was an incorrect change. The `field_type`
variable is already the result of `field.type.upper()` from the line above, so
calling `upper()` again is redundant and would cause an error since we're
trying to call `upper()` on a string that's already uppercase. I'll revert this
change.
##########
sdks/python/apache_beam/io/gcp/bigquery_tools.py:
##########
@@ -384,10 +388,28 @@ def __init__(self, client=None, temp_dataset_id=None,
temp_table_ref=None):
else:
self.temp_table_ref = None
self._temporary_table_suffix = uuid.uuid4().hex
- self.temp_dataset_id = temp_dataset_id or self._get_temp_dataset()
Review Comment:
@stankiewicz You're right, I apologize for the confusion. Let me explain the
temp dataset handling logic and fix my changes:
1. The original code correctly handles three scenarios:
- If `temp_table_ref` is provided, use its dataset ID
- If `temp_dataset_id` is provided, use that
- If neither is provided, generate a temporary dataset ID with
`TEMP_DATASET` prefix and unique suffix
2. My change would break this by removing the default temp dataset creation
when both are None. Here's the corrected version:
if temp_table_ref is not None:
self.temp_table_ref = temp_table_ref
self.temp_dataset_id = temp_table_ref.datasetId
else:
self.temp_table_ref = None
self.temporary_table_suffix = uuid.uuid4().hex
self.temp_dataset_id = temp_dataset_id or (BigQueryWrapper.TEMP_DATASET +
self.temporary_table_suffix)
This preserves the original behavior while keeping the code clean and
readable. The key is using the `or` operator to set `temp_dataset_id` to either:
- The provided `temp_dataset_id` if it exists, or
- A generated temporary ID (TEMP_DATASET + suffix) if `temp_dataset_id` is
None
I'll update the PR with this fix. Thank you for the careful review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
