Skip to content

Conversation

@iambriccardo
Copy link
Contributor

@iambriccardo iambriccardo commented Feb 2, 2026

Summary by CodeRabbit

  • Chores

    • Updated BigQuery dependency revision for enhanced compatibility.
  • Refactor

    • Simplified internal memory management patterns across BigQuery client components to reduce overhead and improve efficiency.

@coderabbitai
Copy link

coderabbitai bot commented Feb 2, 2026

📝 Walkthrough

Walkthrough

This PR removes Arc wrapping from BigQuery client APIs and supporting data structures. It updates method signatures for append_table_batches and create_table_batch to accept and return non-Arc types, updates internal retry logic to match, adds Clone derivation to BigQueryTableRow, and updates a dependency revision.

Changes

Cohort / File(s) Summary
Dependency Update
Cargo.toml
Updated gcp-bigquery-client git revision from 66d97a5 to 2bc93b0.
BigQuery Client API
etl-destinations/src/bigquery/client.rs
Removed Arc wrapping from append_table_batches and create_table_batch; changed parameter from Arc to TableDescriptor and return type from Arc<TableBatch<...>> to TableBatch<...>.
BigQuery Implementation
etl-destinations/src/bigquery/core.rs
Updated prepare_table_for_streaming return type to plain TableDescriptor; refactored append_table_batches_with_retry to work with non-Arc TableBatch slices and direct cloning.
BigQuery Encoding
etl-destinations/src/bigquery/encoding.rs
Added Clone derivation to public struct BigQueryTableRow (#[derive(Debug, Clone)]).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: updating the BigQuery library dependency and removing Arc wrapping from the public API.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch remove-arc-table-descriptor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@iambriccardo iambriccardo changed the title ref(bigquery): Update BigQuery library and remove Arc<TableDescriptor? ref(bigquery): Update BigQuery library and remove Arc<TableDescriptor> Feb 2, 2026
@iambriccardo iambriccardo marked this pull request as ready for review February 2, 2026 08:01
@iambriccardo iambriccardo requested a review from a team as a code owner February 2, 2026 08:01
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
etl-destinations/src/bigquery/client.rs (2)

333-349: ⚠️ Potential issue | 🟡 Minor

Update docs to remove the Arc references.

The function signature now takes owned TableBatch values, but the doc still describes Arc-wrapped batches and non-cloning retries.

📝 Suggested doc update
-    /// Accepts pre-constructed TableBatch objects wrapped in Arc and processes them concurrently
-    /// with controlled parallelism. This allows streaming to multiple different tables efficiently
-    /// in a single call. The Arc wrapping enables efficient retry operations without cloning data.
+    /// Accepts pre-constructed TableBatch objects and processes them concurrently
+    /// with controlled parallelism. This allows streaming to multiple different tables efficiently
+    /// in a single call. Batches are cloned for retry attempts.

As per coding guidelines: “Document all items, public and private, using stdlib tone and precision; only use 'Panics' section when a function can panic.”


414-443: ⚠️ Potential issue | 🟡 Minor

Update docs to match the non-Arc return type.

The doc still states the batch is wrapped in Arc, which is no longer true.

📝 Suggested doc update
-    /// Converts TableRow instances to BigQueryTableRow and creates a properly configured
-    /// TableBatch wrapped in Arc for efficient sharing and retry operations.
+    /// Converts TableRow instances to BigQueryTableRow and creates a properly configured
+    /// TableBatch for streaming inserts.

As per coding guidelines: “Document all items, public and private, using stdlib tone and precision; only use 'Panics' section when a function can panic.”

@coveralls
Copy link

coveralls commented Feb 2, 2026

Pull Request Test Coverage Report for Build 21581975982

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • 102 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.06%) to 81.027%

Files with Coverage Reduction New Missed Lines %
etl/src/pipeline.rs 2 80.82%
etl/src/replication/apply.rs 4 82.93%
etl/src/workers/table_sync.rs 4 88.4%
etl-postgres/src/tokio/test_utils.rs 6 81.55%
etl/src/test_utils/event.rs 8 87.14%
etl/src/test_utils/notifying_store.rs 8 87.1%
etl/src/test_utils/test_destination_wrapper.rs 14 79.12%
etl/src/replication/client.rs 56 79.47%
Totals Coverage Status
Change from base Build 21581693308: 0.06%
Covered Lines: 17488
Relevant Lines: 21583

💛 - Coveralls

@iambriccardo iambriccardo merged commit 6f5de86 into main Feb 2, 2026
14 checks passed
@iambriccardo iambriccardo deleted the remove-arc-table-descriptor branch February 2, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants