Skip to content

test(transaction): Add round-trip tests for Properties & Location#2392

Open
Kurtiscwright wants to merge 2 commits into
apache:mainfrom
Kurtiscwright:transaction-tests-1322
Open

test(transaction): Add round-trip tests for Properties & Location#2392
Kurtiscwright wants to merge 2 commits into
apache:mainfrom
Kurtiscwright:transaction-tests-1322

Conversation

@Kurtiscwright

Copy link
Copy Markdown
Contributor

Adds three end-to-end tests that exercise Transaction::commit against an in-process MemoryCatalog, covering UpdatePropertiesAction, UpdateLocationAction, and a chained transaction that combines both. Each test asserts post-commit metadata state on both the returned Table and a fresh load_table from the catalog.

Also adds a make_v2_minimal_table_in_catalog helper in transaction::tests, parallel to the existing make_v3_minimal_table_in_catalog.

First PR for #1322 next PR will implement tests for (FastAppend beyond test_row_lineage, UpgradeFormatVersion, ReplaceSortOrder, UpdateStatistics) to follow in a separate PR once this PR is merged.

Which issue does this PR close?

  • Closes #.

What changes are included in this PR?

Are these changes tested?

…teLocation

Adds three end-to-end tests that exercise Transaction::commit against an
in-process MemoryCatalog, covering UpdatePropertiesAction,
UpdateLocationAction, and a chained transaction that combines both. Each
test asserts post-commit metadata state on both the returned Table and a
fresh load_table from the catalog.

Also adds a make_v2_minimal_table_in_catalog helper in transaction::tests,
parallel to the existing make_v3_minimal_table_in_catalog.

First PR for apache#1322 next PR will implement tests for
(FastAppend beyond test_row_lineage, UpgradeFormatVersion, ReplaceSortOrder,
UpdateStatistics) to follow in a separate PR once this PR is merged.
@Kurtiscwright Kurtiscwright force-pushed the transaction-tests-1322 branch from d0ae47b to 3ce7a40 Compare May 6, 2026 20:21

@dannycjones dannycjones 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.

LGTM thank you for adding these, this looks like a good test pattern to build on

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions github-actions Bot added the stale label Jul 6, 2026
}

#[tokio::test]
async fn test_chained_actions_single_commit() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the assertions here are insufficient to say that this happened in a "single commit" vs. two independent commits (i.e. this test but rewritten to commit on each tx action)... seems like not the most important behavior to test, but could add something like the following to assert it more directly:

assert_eq!(reloaded.metadata().metadata_log().len(), 1);

but it also feels like we should be testing the case where chained actions cause a single commit to abort. Something like:

#[tokio::test]
async fn test_failing_chained_actions_does_not_commit() {
    // A failed transaction with multiple actions should apply none of the changes.
	// ... same as other tests ...
    let tx = tx
        .update_schema()
        .delete_column("nonexistent".to_string())
        .apply(tx)
        .unwrap();

    // The commit fails and the catalog was not updated.
    assert!(tx.commit(&catalog).await.is_err());
    let reloaded = catalog.load_table(table.identifier()).await.unwrap();
    assert_eq!(reloaded.metadata().properties().get("owner"), None);
    assert_eq!(reloaded.metadata().location(), original_location);
    assert_eq!(reloaded.metadata().metadata_log().len(), 0);
}

Apologies for the drive-by review, trying to familiarize myself with the repo and contribute in some way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants