fix: handle partition overwrite #206
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala (1)
45-52: Remove outdated TODO commentThe TODO references PR #1320 which seems resolved based on the comment.
- ), // todo(tchow): No longer needed after https://github.com/GoogleCloudDataproc/spark-bigquery-connector/pull/1320 + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (2)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala (2)
44-44: LGTM: Table reachability checkGood defensive programming to verify table accessibility before operations.
53-57: Verify partition field behaviorThe conditional inclusion of partitionField could affect existing partition overwrites.
nikhil-zlai
approved these changes
Jan 14, 2025
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/main/scala/ai/chronon/api/DataPointer.scala (1)
18-19: Consider separating read and write options.Currently, both
readOptionsandwriteOptionsreturn the same map. Consider allowing different options for read and write operations.case class URIDataPointer( override val tableOrPath: String, override val readFormat: Option[String], override val writeFormat: Option[String], - options: Map[String, String] + readOptions: Map[String, String], + writeOptions: Map[String, String] ) extends DataPointer { - override val readOptions: Map[String, String] = options - override val writeOptions: Map[String, String] = options }Also applies to: 21-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
api/src/main/scala/ai/chronon/api/DataPointer.scala(1 hunks)api/src/test/scala/ai/chronon/api/test/DataPointerTest.scala(1 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/CatalogAwareDataPointer.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/Extensions.scala(4 hunks)spark/src/main/scala/ai/chronon/spark/TableUtils.scala(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/src/test/scala/ai/chronon/api/test/DataPointerTest.scala
🚧 Files skipped from review as they are similar to previous changes (2)
- cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala
- spark/src/main/scala/ai/chronon/spark/TableUtils.scala
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: other_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (6)
spark/src/main/scala/ai/chronon/spark/CatalogAwareDataPointer.scala (3)
14-16: LGTM! Good use of lazy evaluation.The implementation correctly retrieves read options from the format provider.
18-19: LGTM! Consistent with readOptions implementation.The implementation mirrors readOptions for write operations.
34-34: LGTM! Better method naming.Renaming from
applytofrombetter follows the factory method pattern.api/src/main/scala/ai/chronon/api/DataPointer.scala (1)
9-10: LGTM! Good separation of concerns.Clear distinction between read and write options.
spark/src/main/scala/ai/chronon/spark/Extensions.scala (2)
Line range hint
307-337: LGTM! Clean format handling.Good use of pattern matching and consistent error handling.
Line range hint
346-372: LGTM! Symmetric with save implementation.Consistent pattern matching and error handling with the save method.
tchow-zlai
commented
Jan 14, 2025
| @@ -41,14 +41,16 @@ case class GcpFormatProvider(sparkSession: SparkSession) extends FormatProvider | |||
| override def writeFormat(tableName: String): Format = { | |||
|
|
|||
| val tu = TableUtils(sparkSession) | |||
| val partitionColumnOption = | |||
| if (tu.tableReachable(tableName)) Map.empty else Map("partitionField" -> tu.partitionColumn) | |||
There was a problem hiding this comment.
threading the options through here.
tchow-zlai
commented
Jan 14, 2025
| options: Map[String, String] | ||
| ) extends DataPointer { | ||
|
|
||
| override val readOptions: Map[String, String] = options |
There was a problem hiding this comment.
for URIDataPointer, just treat the options the same.
tchow-zlai
force-pushed
the
tchow/test-bq-connector
branch
from
c7ca79f to
8f95b2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spark/src/main/scala/ai/chronon/spark/CatalogAwareDataPointer.scala (1)
Line range hint
34-46: Add error handling for reflection.The factory method could fail silently. Consider wrapping reflection in Try.
- def from(tableOrPath: String, sparkSession: SparkSession): DataPointer = { + def from(tableOrPath: String, sparkSession: SparkSession): DataPointer = Try { val clazzName = sparkSession.conf.get("spark.chronon.table.format_provider.class", classOf[DefaultFormatProvider].getName) // ... existing code ... CatalogAwareDataPointer(tableOrPath, provider) - } + }.getOrElse(throw new IllegalStateException(s"Failed to initialize FormatProvider for table: $tableOrPath"))cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala (1)
55-56: Track GCS bucket cleanup.TODO indicates this can be simplified after PR GoogleCloudDataproc/spark-bigquery-connector#1320.
Want me to create an issue to track the GCS bucket cleanup?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
.plugin-versions(1 hunks).tool-versions(1 hunks)api/src/main/scala/ai/chronon/api/DataPointer.scala(1 hunks)api/src/test/scala/ai/chronon/api/test/DataPointerTest.scala(1 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/CatalogAwareDataPointer.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/Extensions.scala(4 hunks)spark/src/main/scala/ai/chronon/spark/TableUtils.scala(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/test/scala/ai/chronon/api/test/DataPointerTest.scala
- spark/src/main/scala/ai/chronon/spark/TableUtils.scala
- spark/src/main/scala/ai/chronon/spark/Extensions.scala
- api/src/main/scala/ai/chronon/api/DataPointer.scala
🔇 Additional comments (6)
spark/src/main/scala/ai/chronon/spark/CatalogAwareDataPointer.scala (2)
14-16: LGTM! Clean separation of read options.
18-19: LGTM! Verify partition handling.The separation allows for different handling of partition columns during writes.
✅ Verification successful
Separation of writeOptions is correctly implemented
The design properly supports format-specific partition handling through format providers, with special cases for BigQuery and consistent partition overwrite behavior configured globally.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if writeOptions is used consistently for partition handling rg -p "writeOptions.*partition" --type scalaLength of output: 46
Script:
#!/bin/bash # Find all writeOptions usages rg -p "writeOptions" --type scala -A 3 # Search for partition-related code rg -p "partition.*write|write.*partition" --type scala # Check format provider implementations fd -e scala -x cat {} \; | rg -p "class.*Format.*Provider"Length of output: 3073
.tool-versions (1)
8-8: Verify gcloud SDK version compatibility✅ Verification successful
✓ gcloud SDK version 504.0.1 appears compatible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this version has known issues or is outdated curl -s "https://cloud.google.com/sdk/docs/release-notes.json" | jq '.[] | select(.version | contains("504.0.1"))' # Check for minimum version requirements in project rg -l "google-cloud|gcloud" | xargs rg "minimum|requires|version"Length of output: 4755
.plugin-versions (1)
2-2: Verify plugin commit hashesAlso applies to: 7-7
✅ Verification successful
Plugin commit hashes verified successfully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify commit existence and check if it's the latest stable for repo in "https://github.com/jthegedus/asdf-gcloud.git" "https://github.com/alisaifee/asdf-thrift.git"; do echo "Checking $repo..." git ls-remote $repo HEAD doneLength of output: 685
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala (2)
41-43: LGTM: Clear table ID validation.Assertions properly validate required fields with descriptive error messages.
46-47: Verify table reachability implementation.Logic aligns with PR objective: partition field only specified for new tables.
✅ Verification successful
Table reachability check verified as correct.
Implementation properly attempts to load table and handles failures gracefully. The partition field logic in BigQueryFormat.scala correctly uses this to determine when partition specification is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify TableUtils.tableReachable implementation rg -A 5 "def tableReachable"Length of output: 503
Script:
#!/bin/bash # Find loadTable implementation ast-grep --pattern 'def loadTable($$$)'Length of output: 41
Script:
#!/bin/bash # Find loadTable implementation with broader context rg -A 10 "loadTable"Length of output: 21766
tchow-zlai
force-pushed
the
tchow/test-bq-connector
branch
from
8f95b2b to
5622486
Compare
Co-authored-by: Thomas Chow <[email protected]> Co-authored-by: Thomas Chow <[email protected]> Co-authored-by: Thomas Chow <[email protected]>
Co-authored-by: Thomas Chow <[email protected]> Co-authored-by: Thomas Chow <[email protected]>
Co-authored-by: Thomas Chow <[email protected]> Co-authored-by: Thomas Chow <[email protected]>
tchow-zlai
force-pushed
the
tchow/test-bq-connector
branch
from
604dc27 to
50ad19e
Compare
This was referenced Jan 21, 2025
Merged
This was referenced Feb 12, 2025
Merged
4 tasks
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 25, 2025
## Summary Based on [@david Han](https://zipline-2kh4520.slack.com/team/U0846REC8F7)’s observation's around the spark-bigquery [connector](https://zipline-2kh4520.slack.com/archives/C08710CDH8D/p1736700947319249?thread_ts=1736644291.357239&cid=C08710CDH8D), there indeed is a lurking behavior. when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option. when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec. ^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites. Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced separate read and write options for data pointers - Added support for Google Cloud SDK tool - Added new plugins for `gcloud` and `thrift` - **Bug Fixes** - Enhanced error handling in BigQuery format provider - **Refactor** - Standardized `DataPointer` instantiation method from `apply` to `from` - Improved options handling in data operations - **Chores** - Updated plugin and tool versions for development environment <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209143482009688 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Thomas Chow <[email protected]>
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 29, 2025
## Summary Based on [@david Han](https://zipline-2kh4520.slack.com/team/U0846REC8F7)’s observation's around the spark-bigquery [connector](https://zipline-2kh4520.slack.com/archives/C08710CDH8D/p1736700947319249?thread_ts=1736644291.357239&cid=C08710CDH8D), there indeed is a lurking behavior. when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option. when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec. ^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites. Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced separate read and write options for data pointers - Added support for Google Cloud SDK tool - Added new plugins for `gcloud` and `thrift` - **Bug Fixes** - Enhanced error handling in BigQuery format provider - **Refactor** - Standardized `DataPointer` instantiation method from `apply` to `from` - Improved options handling in data operations - **Chores** - Updated plugin and tool versions for development environment <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209143482009688 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
## Summary Based on [@david Han](https://zipline-2kh4520.slack.com/team/U0846REC8F7)’s observation's around the spark-bigquery [connector](https://zipline-2kh4520.slack.com/archives/C08710CDH8D/p1736700947319249?thread_ts=1736644291.357239&cid=C08710CDH8D), there indeed is a lurking behavior. when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option. when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec. ^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites. Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced separate read and write options for data pointers - Added support for Google Cloud SDK tool - Added new plugins for `gcloud` and `thrift` - **Bug Fixes** - Enhanced error handling in BigQuery format provider - **Refactor** - Standardized `DataPointer` instantiation method from `apply` to `from` - Improved options handling in data operations - **Chores** - Updated plugin and tool versions for development environment <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209143482009688 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
## Summary Based on [@david Han](https://zipline-2kh4520.slack.com/team/U0846REC8F7)’s observation's around the spark-bigquery [connector](https://zipline-2kh4520.slack.com/archives/C08710CDH8D/p1736700947319249?thread_ts=1736644291.357239&cid=C08710CDH8D), there indeed is a lurking behavior. when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option. when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec. ^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites. Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced separate read and write options for data pointers - Added support for Google Cloud SDK tool - Added new plugins for `gcloud` and `thrift` - **Bug Fixes** - Enhanced error handling in BigQuery format provider - **Refactor** - Standardized `DataPointer` instantiation method from `apply` to `from` - Improved options handling in data operations - **Chores** - Updated plugin and tool versions for development environment <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209143482009688 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 16, 2025
## Summary Based on [@david Han](https://zipline-2kh4520.slaour clients.com/team/U0846REC8F7)’s observation's around the spark-bigquery [connector](https://zipline-2kh4520.slaour clients.com/archives/C08710CDH8D/p1736700947319249?thread_ts=1736644291.357239&cid=C08710CDH8D), there indeed is a lurking behavior. when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option. when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec. ^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites. Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases. ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced separate read and write options for data pointers - Added support for Google Cloud SDK tool - Added new plugins for `gcloud` and `thrift` - **Bug Fixes** - Enhanced error handling in BigQuery format provider - **Refactor** - Standardized `DataPointer` instantiation method from `apply` to `from` - Improved options handling in data operations - **Chores** - Updated plugin and tool versions for development environment <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1209143482009688 <!-- av pr metadata This information is embedded by the av CLI when creating PRs to traour clients the status of staour clientss when using Aviator. Please do not delete or edit this section of the PR. ``` {"parent":"main","parentHead":"","trunk":"main"} ``` --> --------- Co-authored-by: Thomas Chow <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Based on
@David Han’s observation's around the spark-bigquery connector, there indeed is a lurking behavior.
when creating BQ tables (in the case they don't exist), user needs to specify a partitioning. This is expected. We do that in the form of a partitionColumn write option.
when the connector performs dynamic partition overwrites, you don't need to specify the partitioning at all. It will do the right thing because the destination table was already created with a partition spec.
^ ideally, the above could be idempotent even if the user passes the partition column to the write, but unfortunately it's a strict requirement that you don't define the partition column when doing dynamic partition overwrites.
Fix is to specify the partition column only when the table DNE and needs to be created, and leave it out in all other cases.
Checklist
Summary by CodeRabbit
New Features
gcloudandthriftBug Fixes
Refactor
DataPointerinstantiation method fromapplytofromChores