Skip to content

feat(iceberg): More flexible IcebergClient constructor. #540

Open
andyyu2004 wants to merge 1 commit intosupabase:mainfrom
andyyu2004:raw-catalog-constructor
Open

feat(iceberg): More flexible IcebergClient constructor. #540
andyyu2004 wants to merge 1 commit intosupabase:mainfrom
andyyu2004:raw-catalog-constructor

Conversation

@andyyu2004
Copy link

@andyyu2004 andyyu2004 commented Jan 12, 2026

What kind of change does this PR introduce?

Feature.

Additional context

The existing IcebergClient constructors are not flexible enough for all use cases.

This changes introduces a less opinionated constructor IcebergClient::new: fn(Arc<dyn Catalog>) -> Self.

I've omitted creating an issue as it's a very small change.

Summary by CodeRabbit

  • Refactor
    • Improved internal initialization of the Iceberg catalog to reduce duplication and unify creation across different catalog implementations. This enhances maintainability and consistency of catalog handling with no user-facing behavior changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@andyyu2004 andyyu2004 requested a review from a team as a code owner January 12, 2026 00:00
@andyyu2004 andyyu2004 force-pushed the raw-catalog-constructor branch from 9524c99 to 3542700 Compare January 14, 2026 01:39
@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

Added a public constructor IcebergClient::new(Arc<dyn Catalog>). Two existing constructors (new_with_rest_catalog and new_with_supabase_catalog) were refactored to call this new constructor, centralizing client construction.

Changes

Cohort / File(s) Change Summary
IcebergClient Constructor Refactoring
etl-destinations/src/iceberg/client.rs
Added public new(Arc<dyn Catalog>) constructor. Refactored new_with_rest_catalog() and new_with_supabase_catalog() to delegate to the new constructor, removing duplicated construction logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 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 'More flexible IcebergClient constructor' directly matches the main change: adding a new public constructor that accepts Arc to provide more flexibility than existing constructors.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3542700 and 1f1a17e.

📒 Files selected for processing (1)
  • etl-destinations/src/iceberg/client.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use snake_case for files and modules
Use CamelCase for types and traits
Use snake_case for functions and variables
Do not leave comments when you remove things
Document all items, public and private, using stdlib tone and precision; only use 'Panics' section when a function can panic
Link types and methods as [Type], [Type::method] in Rust documentation comments
Keep Rust documentation wording concise, correct, and punctuated; reword for clarity while preserving intent
Do not include code examples in Rust documentation; include private helpers for maintainers; apply documentation to modules, types, traits, impls, and functions
Normal comments in Rust should always finish with a period

Files:

  • etl-destinations/src/iceberg/client.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: depthfirst Bot
🔇 Additional comments (3)
etl-destinations/src/iceberg/client.rs (3)

58-61: LGTM!

The new constructor is well-implemented and the documentation has been added as previously requested. The single-line doc comment is concise and accurate, following the stdlib tone. The implementation correctly wraps the catalog in the struct.


79-79: LGTM!

Good refactoring to use the centralized constructor, eliminating direct struct construction and ensuring consistent client initialization.


122-122: LGTM!

Consistent refactoring that aligns with the new_with_rest_catalog changes. The SupabaseCatalog is correctly wrapped in an Arc for the unified constructor.


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.

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: 1

🤖 Fix all issues with AI agents
In `@etl-destinations/src/iceberg/client.rs`:
- Around line 58-62: The public constructor IcebergClient::new lacks a doc
comment; add a Rust doc comment (///) above pub fn new describing that it
creates a new IcebergClient from a shared Catalog (Arc<dyn Catalog>), document
the parameter `catalog` and the returned IcebergClient, and mirror the style and
detail of the existing constructors new_with_rest_catalog and
new_with_supabase_catalog so the API docs are consistent.
📜 Review details

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f238781 and 3542700.

📒 Files selected for processing (1)
  • etl-destinations/src/iceberg/client.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use snake_case for files and modules
Use CamelCase for types and traits
Use snake_case for functions and variables
Do not leave comments when you remove things
Document all items, public and private, using stdlib tone and precision; only use 'Panics' section when a function can panic
Link types and methods as [Type], [Type::method] in Rust documentation comments
Keep Rust documentation wording concise, correct, and punctuated; reword for clarity while preserving intent
Do not include code examples in Rust documentation; include private helpers for maintainers; apply documentation to modules, types, traits, impls, and functions
Normal comments in Rust should always finish with a period

Files:

  • etl-destinations/src/iceberg/client.rs
🧬 Code graph analysis (1)
etl-destinations/src/iceberg/client.rs (2)
etl-destinations/src/iceberg/core.rs (1)
  • new (170-184)
etl-destinations/src/iceberg/catalog.rs (2)
  • new (37-39)
  • new (191-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: depthfirst Bot
🔇 Additional comments (2)
etl-destinations/src/iceberg/client.rs (2)

80-80: LGTM!

The refactoring correctly delegates to the new constructor, reducing duplication while maintaining the same behavior.


123-123: LGTM!

Consistent refactoring that mirrors the change in new_with_rest_catalog, properly delegating to the new constructor.

@andyyu2004 andyyu2004 force-pushed the raw-catalog-constructor branch from 3542700 to 1f1a17e Compare January 14, 2026 02:11
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.

1 participant