Skip to content

feature(encryption): [17/N] Enable encrypted writes - #3018

Open
xanderbailey wants to merge 3 commits into
apache:mainfrom
xanderbailey:xb/enable_encrypted_writes
Open

feature(encryption): [17/N] Enable encrypted writes#3018
xanderbailey wants to merge 3 commits into
apache:mainfrom
xanderbailey:xb/enable_encrypted_writes

Conversation

@xanderbailey

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

Since merging #2701 we can now write encrypted parquet files which means we now fully support read and write paths. The last step is to remove the error on the commit path to allow users to actually write encrypted tables.

Are these changes tested?

I've added roundtrip tests for datafusion in a new slt test and purge table catalog tests can now be added since we can commit to an encrypted table.

AI Disclosure

@xanderbailey
xanderbailey force-pushed the xb/enable_encrypted_writes branch 2 times, most recently from 3456935 to c38af67 Compare August 18, 2026 13:41
@xanderbailey
xanderbailey force-pushed the xb/enable_encrypted_writes branch from c38af67 to 271c4db Compare August 18, 2026 13:50

@blackmwk blackmwk 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 @xanderbailey for this pr! Generally LGTM, just one comment about integration test.

#[case::sql_catalog(CatalogKind::Sql)]
#[case::memory_catalog(CatalogKind::Memory)]
#[tokio::test]
async fn test_catalog_purge_encrypted_table(#[case] kind: CatalogKind) -> Result<()> {

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.

We have several methods for updating table, I'm thinking maybe we should make encrypted or not a test case param?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gave it a go 85bc744 for the purge test, were there others you think I needed to include in this PR?

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.

Add Encryption purge table tests for catalog

2 participants