Skip to content

feat: add builders for CreateExternalCatalog and DropCatalog - #25785

Merged
alamb merged 2 commits into
apache:mainfrom
TTyChud:feat/catalog-builders
Oct 1, 2026
Merged

alamb merged 2 commits into
apache:mainfrom
TTyChud:feat/catalog-builders

Conversation

@TTyChud

@TTyChud TTyChud commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

These two structs can only be built by setting all seven fields by hand, including
a schema field that is always empty. The builders let you set only what you
need, the same way CreateExternalTable::builder already does.

What changes are included in this PR?

  • Adds CreateExternalCatalog::builder and DropCatalog::builder, with must_use
    with_* setters and build().
  • Uses them in CREATE EXTERNAL CATALOG, DROP CATALOG and DROP DATABASE
    (datafusion/sql/src/statement.rs).

What is the testing strategy for this PR?

  • 5 unit tests in datafusion/expr/src/logical_plan/ddl.rs covering the builder
    defaults, every with_* setter, and that with_location(None) unsets the location.
  • New doc examples on both builders, run by cargo test -p datafusion-expr --doc.
  • cargo fmt --all -- --check, cargo test -p datafusion-expr --lib ddl::test and
    cargo clippy -p datafusion-expr -p datafusion-sql --all-targets --no-deps -- -D warnings
    pass locally.
  • Existing catalog tests are unchanged.

Are there any user-facing changes?

Two new public builders. Nothing removed or changed.

@github-actions github-actions Bot added sql SQL Planner logical-expr Logical plan and expressions labels Sep 27, 2026
@TTyChud
TTyChud force-pushed the feat/catalog-builders branch from 4cac198 to 8006aed Compare September 27, 2026 01:40

@martin-g martin-g left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add some unit tests for the new builders.

Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Comment thread datafusion/expr/src/logical_plan/ddl.rs
Adds builder APIs for the two catalog DDL structs, mirroring the
CreateExternalTable builder added in apache#19066, and rewires the CREATE EXTERNAL
CATALOG and DROP CATALOG/DROP DATABASE construction sites in datafusion-sql.

Closes apache#25627.
@TTyChud
TTyChud force-pushed the feat/catalog-builders branch from 8006aed to c9a020e Compare October 1, 2026 06:11
@TTyChud
TTyChud requested a review from martin-g October 1, 2026 06:13
@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.58%. Comparing base (84b84e5) to head (7ead6c0).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #25785    +/-   ##
========================================
  Coverage   82.58%   82.58%            
========================================
  Files        1145     1145            
  Lines      443655   443783   +128     
  Branches   443655   443783   +128     
========================================
+ Hits       366401   366513   +112     
- Misses      55004    55018    +14     
- Partials    22250    22252     +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@TTyChud
TTyChud requested a review from martin-g October 1, 2026 12:28
@TTyChud

TTyChud commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

I dont think there's a problem with my code can u plz rerun it

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @TTyChud and @martin-g -- this looks great to me

)),
ObjectType::Database => {
Ok(LogicalPlan::Ddl(DdlStatement::DropCatalog(
DropCatalog::builder(object_name_to_string(&name))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@alamb
alamb enabled auto-merge October 1, 2026 17:38
@alamb
alamb added this pull request to the merge queue Oct 1, 2026
Merged via the queue into apache:main with commit 141663e Oct 1, 2026
42 checks passed
Omega359 pushed a commit to Omega359/arrow-datafusion that referenced this pull request Oct 11, 2026
…che#25785)

## Which issue does this PR close?

- Closes apache#25627.

## Rationale for this change

These two structs can only be built by setting all seven fields by hand,
including
a `schema` field that is always empty. The builders let you set only
what you
need, the same way `CreateExternalTable::builder` already does.

## What changes are included in this PR?

- Adds `CreateExternalCatalog::builder` and `DropCatalog::builder`, with
`must_use`
  `with_*` setters and `build()`.
- Uses them in `CREATE EXTERNAL CATALOG`, `DROP CATALOG` and `DROP
DATABASE`
  (`datafusion/sql/src/statement.rs`).

## What is the testing strategy for this PR?

- 5 unit tests in `datafusion/expr/src/logical_plan/ddl.rs` covering the
builder
defaults, every `with_*` setter, and that `with_location(None)` unsets
the location.
- New doc examples on both builders, run by `cargo test -p
datafusion-expr --doc`.
- `cargo fmt --all -- --check`, `cargo test -p datafusion-expr --lib
ddl::test` and
`cargo clippy -p datafusion-expr -p datafusion-sql --all-targets
--no-deps -- -D warnings`
  pass locally.
- Existing catalog tests are unchanged.

## Are there any user-facing changes?

Two new public builders. Nothing removed or changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions sql SQL Planner v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a builder for CreateExternalCatalog and DropCatalog

4 participants