Skip to content

fix(catalog-sql): forward catalog props to FileIO#2528

Open
kszucs wants to merge 1 commit into
apache:mainfrom
kszucs:fix-sql-catalog-fileio-props
Open

fix(catalog-sql): forward catalog props to FileIO#2528
kszucs wants to merge 1 commit into
apache:mainfrom
kszucs:fix-sql-catalog-fileio-props

Conversation

@kszucs
Copy link
Copy Markdown
Member

@kszucs kszucs commented May 29, 2026

Which issue does this PR close?

  • Closes #.

What changes are included in this PR?

Storage-backend keys set on SqlCatalog (e.g. authentication tokens, region overrides) were not reaching the FileIO, because SqlCatalog::new built the FileIO from the StorageFactory alone, dropping the props on the floor. Authenticated storage backends then saw empty config and rejected writes with 401s.

The fix forwards the catalog props into the FileIO:

let fileio = FileIOBuilder::new(factory)
    .with_props(config.props.clone())
    .build();

This brings SqlCatalog into parity with the RestCatalog, which already forwards props through FileIOBuilder::with_props. Storage backends ignore keys they don't recognize, so catalog-only props (pool.* etc.) remain harmless.

Are these changes tested?

Yes — added unit test test_storage_props_propagate_to_file_io in crates/catalog/sql/src/catalog.rs which:

  • Builds a SqlCatalog with two storage-backend props (one S3-shaped, one HF-shaped) in the load map.
  • Asserts both keys are present in catalog.fileio.config().props().

The test fails on main (the props lookup returns None) and passes with this change. Verified locally by reverting the one-line change in the source, observing the assertion failure, then restoring.

Storage-backend keys set on SqlCatalog were not reaching the FileIO
because SqlCatalog::new built it without forwarding props, causing
401s on authenticated writes. Matches the RestCatalog wiring.
@kszucs kszucs force-pushed the fix-sql-catalog-fileio-props branch from 4941490 to 65eb9b9 Compare May 29, 2026 12:26
Copy link
Copy Markdown
Collaborator

@CTTY CTTY left a comment

Choose a reason for hiding this comment

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

LGTM! Just one nit

}

/// Regression test: storage-backend props set on the catalog must reach
/// the FileIO; otherwise authenticated backends fail with 401s on writes.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this should not be rustdoc? I think a normal // would be better

Copy link
Copy Markdown
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

LGTM pending fix for Shawn's comment.

I'd love to see this one merged for 0.10 (#2527).

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.

3 participants