Revert "chore: Remove use of DelegatingTable and bubble up exceptions properly" #661
Conversation
… properl…" This reverts commit 6b482ea.
WalkthroughThis change introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Catalog as DelegatingBigQueryMetastoreCatalog
participant Iceberg
participant BQ as BigQuery
participant DT as DelegatingTable
Caller->>Catalog: loadTable(ident)
Catalog->>Iceberg: try loadTable(ident)
alt Iceberg table found
Iceberg-->>Catalog: Table
Catalog->>DT: wrap Table with provider=ICEBERG, external=false
DT-->>Catalog: DelegatingTable
else Iceberg table not found
Catalog->>BQ: getTable(ident)
alt ExternalTable
BQ-->>Catalog: ExternalTableDefinition
Catalog->>DT: wrap ParquetTable with provider=PARQUET, external=true, location
DT-->>Catalog: DelegatingTable
else StandardTable
BQ-->>Catalog: StandardTableDefinition
Catalog->>DT: wrap Table with provider=BIGQUERY, external=false
DT-->>Catalog: DelegatingTable
end
end
Catalog-->>Caller: DelegatingTable
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. 🪧 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: 3
🧹 Nitpick comments (2)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/DelegatingBigQueryMetastoreCatalog.scala (1)
56-58: Clarify property precedence.
internalTable.properties ++ additionalPropertieslets extras override clashes (good).
A comment here would save readers an extra mental hop.cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/GcpFormatProvider.scala (1)
29-41: Swallowing errors hides root cause.
Try(...).toOptionreturnsNoneon any error, losing details.
ConsiderrecoverWiththat logs or re‑raises unsupported provider issues.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: service_tests
- GitHub Check: hub_tests
- GitHub Check: service_tests
- GitHub Check: service_commons_tests
- GitHub Check: hub_tests
- GitHub Check: online_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: flink_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: online_tests
- GitHub Check: flink_tests
- GitHub Check: api_tests
- GitHub Check: api_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: aggregator_tests
- GitHub Check: aggregator_tests
| override def newScanBuilder(options: CaseInsensitiveStringMap): ScanBuilder = | ||
| internalTable.asInstanceOf[SupportsRead].newScanBuilder(options) | ||
|
|
||
| override def newWriteBuilder(info: LogicalWriteInfo): WriteBuilder = | ||
| internalTable.asInstanceOf[SupportsWrite].newWriteBuilder(info) | ||
|
|
There was a problem hiding this comment.
Guard the casts to SupportsRead/Write.
asInstanceOf will blow up if internalTable lacks SupportsRead/SupportsWrite.
Prefer pattern‑match or collect { case t: SupportsRead => … } to fail fast with a clearer message.
| Try { | ||
| icebergCatalog.loadTable(identNoCatalog) | ||
| val icebergSparkTable = icebergCatalog.loadTable(identNoCatalog) | ||
| DelegatingTable(icebergSparkTable, | ||
| additionalProperties = | ||
| Map(TableCatalog.PROP_EXTERNAL -> "false", TableCatalog.PROP_PROVIDER -> "ICEBERG")) | ||
| } | ||
| .recover { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Too broad fallback.
recover { case _ => … } hides non‑existence and real errors alike.
Match _: NoSuchTableException explicitly to avoid masking genuine failures.
| val table = bigQueryClient.getTable(tId) | ||
| table.getDefinition.asInstanceOf[TableDefinition] match { |
There was a problem hiding this comment.
Null‑safe BigQuery lookup needed.
getTable returns null when absent → NPE later.
Wrap with Option(...) and throw NoSuchTableException early.
- val table = bigQueryClient.getTable(tId)
+ val table = Option(bigQueryClient.getTable(tId))
+ .getOrElse(throw new NoSuchTableException(s"Table $tId not found in BigQuery"))
Reverts #638
Summary by CodeRabbit
New Features
Bug Fixes
Refactor