feat: Allow configuring dataset location in BigQueryCache#689
Conversation
Jan Bílek (honzabilek4)
force-pushed
the
main
branch
from
715712e to
5267b78
Compare
📝 Walkthrough""" WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BigQueryCache
participant DestinationBigquery
Caller->>BigQueryCache: Access project_name, dataset_name, dataset_location, credentials_path
Caller->>DestinationBigquery: Create with project_id, dataset_id, dataset_location, credentials_json, loading_method
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Would you like me to help you draft a quick test or documentation update for this new 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Jan Bílek (honzabilek4)
force-pushed
the
main
branch
from
5267b78 to
00dada2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
airbyte/_processors/sql/bigquery.py (1)
50-52: Nice addition! Just a small formatting fix needed.The new
dataset_locationfield looks great - good default value, proper typing, and helpful documentation linking to the official BigQuery locations. One small thing though - there's trailing whitespace on line 51 that's causing the linter to complain. Could you remove it? wdyt?dataset_location: str = "US" - """The geographic location of the BigQuery dataset (e.g., 'US', 'EU', etc.). + """The geographic location of the BigQuery dataset (e.g., 'US', 'EU', etc.). Defaults to 'US'. See: https://cloud.google.com/bigquery/docs/locations"""🧰 Tools
🪛 Ruff (0.11.9)
51-51: Trailing whitespace
Remove trailing whitespace
(W291)
🪛 GitHub Actions: Run Linters
[warning] 51-51: Ruff W291: Trailing whitespace detected. Remove trailing whitespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/_processors/sql/bigquery.py(1 hunks)airbyte/destinations/_translate_cache_to_dest.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte/destinations/_translate_cache_to_dest.py (1)
airbyte/_processors/sql/bigquery.py (2)
project_name(55-57)dataset_name(60-62)
🪛 GitHub Actions: Run Linters
airbyte/destinations/_translate_cache_to_dest.py
[warning] 4-22: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
airbyte/_processors/sql/bigquery.py
[warning] 51-51: Ruff W291: Trailing whitespace detected. Remove trailing whitespace.
🪛 Ruff (0.11.9)
airbyte/_processors/sql/bigquery.py
51-51: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (1)
airbyte/destinations/_translate_cache_to_dest.py (1)
130-130: Great integration of the configurable dataset location!I love how you've replaced the hardcoded
"US"withDatasetLocation(cache.dataset_location)- this properly uses the new configurable field from the BigQueryConfig. TheDatasetLocationwrapper appears to be the correct type expected by the API, and sincecache.dataset_locationhas a default value of "US" in the config, this should be safe. Nice work!
Jan Bílek (honzabilek4)
force-pushed
the
main
branch
from
00dada2 to
4f5e6c5
Compare
Jan Bílek (honzabilek4)
force-pushed
the
main
branch
from
4f5e6c5 to
0a68ca3
Compare
|
Hi Aaron ("AJ") Steers (@aaronsteers), |
|
/test-pr
|
|
Jan Bílek (@honzabilek4) - On first review, this looks great! Have you been able to test manually with any success? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/_processors/sql/bigquery.py(3 hunks)airbyte/destinations/_translate_dest_to_cache.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte/destinations/_translate_dest_to_cache.py
🧰 Additional context used
🪛 Ruff (0.11.9)
airbyte/_processors/sql/bigquery.py
211-211: Line too long (106 > 100)
(E501)
212-212: Change outer quotes to avoid escaping inner quotes
Change outer quotes to avoid escaping inner quotes
(Q003)
🪛 GitHub Actions: Run Linters
airbyte/_processors/sql/bigquery.py
[error] 211-211: E501 Line too long (106 > 100)
[error] 212-212: Q003 Change outer quotes to avoid escaping inner quotes
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (2)
airbyte/_processors/sql/bigquery.py (2)
50-53: Great addition for EU compliance requirements!The new
dataset_locationfield perfectly addresses the data residency requirements mentioned in the PR discussion. The default value maintains backward compatibility while the documentation clearly explains the purpose. wdyt?
97-97: Nice integration with BigQuery client initialization!Passing the
dataset_locationto the BigQuery client ensures consistency across operations. This looks spot on for the feature requirements.
|
Aaron ("AJ") Steers (@aaronsteers) – I’ve tested this locally and made a few additional changes to fully resolve the feature. When setting the region, it must be used consistently across all steps. If not, some jobs can fail due to region mismatches. It’s now working correctly. |
|
/test-pr
|
Aaron ("AJ") Steers (aaronsteers)
left a comment
Aaron ("AJ") Steers (aaronsteers)
left a comment
There was a problem hiding this comment.
This is a fantastic contribution. Thank you!!
|
One test failed, but it looks unrelated. It’s testing the MotherDuck connection. |
|
Aaron ("AJ") Steers (@aaronsteers) Any idea what's happening here with the test? Is it flaky? |
Jan Bílek (@honzabilek4) - Apologies for the delay. This test is not specifically known to be flaky, but it is most likely an interim issue that is now resolved. I have some time today to rerun and review more closely. Will merge and release if all looks good. |
Aaron ("AJ") Steers (aaronsteers)
merged commit 345d648
into
airbytehq:main
2 participants
Resolves #688.
Summary by CodeRabbit