chore: Remove use of DelegatingTable and bubble up exceptions properly #638
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
Contributor
WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DelegatingBigQueryMetastoreCatalog
participant IcebergCatalog
participant BigQueryClient
participant ConnectorCatalog
Client->>DelegatingBigQueryMetastoreCatalog: loadTable(identifier)
alt Table in IcebergCatalog
DelegatingBigQueryMetastoreCatalog->>IcebergCatalog: loadTable(identifier)
IcebergCatalog-->>DelegatingBigQueryMetastoreCatalog: Table
DelegatingBigQueryMetastoreCatalog-->>Client: Table
else Table not in IcebergCatalog
DelegatingBigQueryMetastoreCatalog->>BigQueryClient: getTable(...)
alt Table found in BigQuery
DelegatingBigQueryMetastoreCatalog->>ConnectorCatalog: loadTable(...)
ConnectorCatalog-->>DelegatingBigQueryMetastoreCatalog: Table
DelegatingBigQueryMetastoreCatalog-->>Client: Table
else Table not found
DelegatingBigQueryMetastoreCatalog-->>Client: throw NoSuchTableException
end
end
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
🔇 Additional comments (7)
🪧 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala (1)
117-122: Basic BigQuery write test added, consider enhancing.Test validates writing to BigQuery native tables, but lacks assertions to verify data integrity.
it should "writes to bigquery native" in { val nativeTable = "default_iceberg.data.checkouts_native" val df = tableUtils.scanDf(null, nativeTable).limit(10) + val rowCountBefore = tableUtils.loadTable(nativeTable).count() tableUtils.insertPartitions(df, nativeTable) + val rowCountAfter = tableUtils.loadTable(nativeTable).count() + assert(rowCountAfter == rowCountBefore + 10, "Expected 10 new rows after insertion") }cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala (1)
29-36: Added fallback for table property retrieval.Now checks OpenLineage storage layer when provider not found, improving format detection reliability.
val tblProps = tbl.properties() tblProps.asScala .getOrElse(TableCatalog.PROP_PROVIDER, - tblProps.asScala.getOrElse("openlineage.dataset.storageDatasetFacet.storageLayer", "")) + tblProps.asScala.getOrElse("openlineage.dataset.storageDatasetFacet.storageLayer", ""))Consider extracting repeated
tblProps.asScalato a variable to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala(3 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala(1 hunks)cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala (1)
spark/src/main/scala/ai/chronon/spark/TableUtils.scala (2)
scanDf(593-613)insertPartitions(232-283)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala (2)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala (1)
loadTable(79-133)spark/src/main/scala/ai/chronon/spark/TableUtils.scala (1)
loadTable(118-120)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (5)
cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala (1)
44-52: Uncommented necessary configuration for BigQuery integration.Re-enabling these settings with specific GCP project details and catalog implementations.
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala (4)
12-13: Added necessary imports for catalog and exceptions.Added BigQueryMetastoreCatalog and NoSuchTableException imports.
Also applies to: 15-16
79-82: Simplified table loading from Iceberg catalog.Removed DelegatingTable wrapper, directly returning the Iceberg table.
106-118: Streamlined external table creation.Direct ParquetTable creation without intermediate delegation.
126-127: Simplified BigQuery connector table lookup.Direct connector table return without delegation layer.
tchow-zlai
force-pushed
the
tchow/bq-native-through-catalog
branch
from
7e920a8 to
c9fa87e
Compare
tchow-zlai
commented
Apr 14, 2025
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala (1)
118-125: Consider verifying the written rows. A quick check ensures data correctness.+val rowCount = tableUtils.loadTable(newNativeTable).count() +assert(rowCount == 10, s"Expected 10 rows but got $rowCount")cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryConnectorForks.scala (1)
40-62: Mind reflection. Private field access may break with future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
cloud_gcp/BUILD.bazel(1 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryConnectorForks.scala(1 hunks)cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala(2 hunks)maven_install.json(53 hunks)spark/src/main/scala/ai/chronon/spark/TableUtils.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/format/CreationUtils.scala(1 hunks)tools/build_rules/dependencies/maven_repository.bzl(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- cloud_gcp/BUILD.bazel
- tools/build_rules/dependencies/maven_repository.bzl
- spark/src/main/scala/ai/chronon/spark/format/CreationUtils.scala
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: streaming_tests
- GitHub Check: spark_tests
- GitHub Check: streaming_tests
- GitHub Check: join_tests
- GitHub Check: spark_tests
- GitHub Check: groupby_tests
- GitHub Check: join_tests
- GitHub Check: groupby_tests
- GitHub Check: fetcher_tests
- GitHub Check: fetcher_tests
- GitHub Check: batch_tests
- GitHub Check: batch_tests
- GitHub Check: analyzer_tests
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: analyzer_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (7)
maven_install.json (1)
1-34262: All changes are auto-generated dependency additions. No issues—looks good!spark/src/main/scala/ai/chronon/spark/TableUtils.scala (1)
277-280: Looks good. Additional BigQuery write settings are aligned with the rest of the code.cloud_gcp/src/test/scala/ai/chronon/integrations/cloud_gcp/BigQueryCatalogTest.scala (1)
44-53: Config looks correct. These properties properly enable BigQuery usage.cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryConnectorForks.scala (4)
1-2: No remarks.
3-16: No remarks.
17-26: Code is straightforward.
28-38: Implementation merges properties neatly.
tchow-zlai
force-pushed
the
tchow/bq-native-through-catalog
branch
from
03208ad to
c5b6525
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala (1)
133-137: Redundant error handling pattern.The pattern of recovering with Try followed by pattern matching on Success/Failure is redundant.
- } match { - case Success(table) => table - case Failure(exception) => throw exception - } + }.get
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala(4 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: service_commons_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: service_tests
- GitHub Check: hub_tests
- GitHub Check: hub_tests
- GitHub Check: service_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: api_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: online_tests
- GitHub Check: flink_tests
- GitHub Check: aggregator_tests
- GitHub Check: online_tests
- GitHub Check: flink_tests
- GitHub Check: api_tests
- GitHub Check: aggregator_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (6)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala (6)
12-12: Import adjustments for the new implementation.Updated imports to support Try-based error handling and better exception management.
Also applies to: 25-26
81-81: Direct table return without wrapping.Now correctly returns Iceberg table directly without DelegatingTable wrapper.
84-96: Improved BigQuery table retrieval with safer null handling.Now uses Scala Option for safer handling of potentially non-existent tables.
108-119: Direct ParquetTable creation.External table handling now directly returns ParquetTable without unnecessary wrapper.
128-128: Direct connector table return.Returns connector table directly without wrapping.
130-130: Better error message for unsupported tables.More descriptive error message for unsupported table types.
tchow-zlai
force-pushed
the
tchow/bq-native-through-catalog
branch
2 times, most recently
from
6ff8ccc to
6d817ec
Compare
tchow-zlai
changed the title
Co-authored-by: Thomas Chow <[email protected]>
tchow-zlai
force-pushed
the
tchow/bq-native-through-catalog
branch
from
12910df to
3fa0959
Compare
david-zlai
approved these changes
Apr 17, 2025
4 tasks
tchow-zlai
added a commit
that referenced
this pull request
Apr 17, 2025
## Summary - This change got left out of #638, we need to protect against NoSuchTableException now. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
4 tasks
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 25, 2025
#638) ## Summary - It's better to return the underlying table, that way we can let Spark understand what capabilities are possible based on class type matching. - Also modify the catalog to conform to the contract, throwing NoSuchTableException. Improve logging in TableUtils. ## 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 - **Refactor** - Simplified and streamlined table loading and error handling for BigQuery and external tables. - Improved reliability by updating exception handling and type-based logic for table formats. - **Chores** - Removed unused classes, objects, and imports to reduce code complexity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- 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 25, 2025
## Summary - This change got left out of #638, we need to protect against NoSuchTableException now. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
4 tasks
kumar-zlai
pushed a commit
that referenced
this pull request
Apr 29, 2025
#638) ## Summary - It's better to return the underlying table, that way we can let Spark understand what capabilities are possible based on class type matching. - Also modify the catalog to conform to the contract, throwing NoSuchTableException. Improve logging in TableUtils. ## 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 - **Refactor** - Simplified and streamlined table loading and error handling for BigQuery and external tables. - Improved reliability by updating exception handling and type-based logic for table formats. - **Chores** - Removed unused classes, objects, and imports to reduce code complexity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- 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 - This change got left out of #638, we need to protect against NoSuchTableException now. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
#638) ## Summary - It's better to return the underlying table, that way we can let Spark understand what capabilities are possible based on class type matching. - Also modify the catalog to conform to the contract, throwing NoSuchTableException. Improve logging in TableUtils. ## 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 - **Refactor** - Simplified and streamlined table loading and error handling for BigQuery and external tables. - Improved reliability by updating exception handling and type-based logic for table formats. - **Chores** - Removed unused classes, objects, and imports to reduce code complexity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- 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 - This change got left out of #638, we need to protect against NoSuchTableException now. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 15, 2025
#638) ## Summary - It's better to return the underlying table, that way we can let Spark understand what capabilities are possible based on class type matching. - Also modify the catalog to conform to the contract, throwing NoSuchTableException. Improve logging in TableUtils. ## 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 - **Refactor** - Simplified and streamlined table loading and error handling for BigQuery and external tables. - Improved reliability by updating exception handling and type-based logic for table formats. - **Chores** - Removed unused classes, objects, and imports to reduce code complexity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- 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 - This change got left out of #638, we need to protect against NoSuchTableException now. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 16, 2025
#638) ## Summary - It's better to return the underlying table, that way we can let Spark understand what capabilities are possible based on class type matching. - Also modify the catalog to conform to the contract, throwing NoSuchTableException. Improve logging in TableUtils. ## 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 - **Refactor** - Simplified and streamlined table loading and error handling for BigQuery and external tables. - Improved reliability by updating exception handling and type-based logic for table formats. - **Chores** - Removed unused classes, objects, and imports to reduce code complexity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- 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]>
chewy-zlai
pushed a commit
that referenced
this pull request
May 16, 2025
## Summary - This change got left out of #638, we need to protect against NoSuchTableException now. ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- 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"} ``` --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling to prevent issues when accessing unreachable tables, ensuring the system returns an empty result instead of failing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
4 tasks
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
Checklist
Summary by CodeRabbit