Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for using JDBC Catalog for Iceberg testing #6144

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

devinrsmith
Copy link
Member

Fixes #6029

@devinrsmith devinrsmith added this to the 0.37.0 milestone Sep 26, 2024
@devinrsmith devinrsmith self-assigned this Sep 26, 2024
Comment on lines +82 to +84
final TableDefinition td = catalogAdapter.getTableDefinition(CITIES_ID.toString(), SNAPSHOT_1_ID, null);
assertThat(td).isEqualTo(CITIES_1_TD);
cities1 = catalogAdapter.readTable(CITIES_ID, SNAPSHOT_1_ID);
Copy link
Member Author

@devinrsmith devinrsmith Sep 26, 2024

Choose a reason for hiding this comment

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

Note to @lbooker42 , the mismatch between signature of getTableDefinition and readTable.

lbooker42
lbooker42 previously approved these changes Sep 27, 2024
Copy link
Contributor

@lbooker42 lbooker42 left a comment

Choose a reason for hiding this comment

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

LGTM

extensions/iceberg/TESTING.md Outdated Show resolved Hide resolved
Comment on lines +4 to +5
package io.deephaven.iceberg.junit5;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use Junit4 instead of Junit5, because for my case, I will be writing deephaven tables to iceberg, and for creating these tables, I would need an execution context which so far I have been using with Junit4 using

    @Rule
    public final EngineCleanup framework = new EngineCleanup();

Let me know if I am missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be super simple to create JUnit 5 tests using the same logic; I don't think it's unreasonable to do this:

    private EngineCleanup engineCleanup = new EngineCleanup();

    @BeforeEach
    void setUp() throws Exception {
        engineCleanup.setUp();
    }

    @AfterEach
    void tearDown() throws Exception {
        engineCleanup.tearDown();
    }
}

We could create a JUnit 5 extension to make it just as easy as @Rule (I think I've done this on some branch, but I forget where...).

I really really really think we should be preferring JUnit 5 for new tests going forward.

I've updated io.deephaven.iceberg.junit5.CatalogAdapterBase to use EngineCleanup in anticipation of the ticking testing that should be upcoming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks!

@devinrsmith devinrsmith enabled auto-merge (squash) September 27, 2024 19:15
@devinrsmith devinrsmith merged commit ff1621a into deephaven:main Sep 27, 2024
16 checks passed
@devinrsmith devinrsmith deleted the iceberg-jdbc-testing branch September 27, 2024 19:43
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Iceberg CI testing by leveraging standard catalogs
3 participants