Skip to content

Core: Clean up JDBC implementation, add catalog namespace tests#4231

Merged
rdblue merged 5 commits into
apache:masterfrom
rdblue:add-catalog-namespace-tests
Feb 25, 2022
Merged

Core: Clean up JDBC implementation, add catalog namespace tests#4231
rdblue merged 5 commits into
apache:masterfrom
rdblue:add-catalog-namespace-tests

Conversation

@rdblue

@rdblue rdblue commented Feb 25, 2022

Copy link
Copy Markdown
Contributor

This continues cleaning up the JDBC implementation after #4220 and adds namespace tests to validate the behavior of the JDBC catalog and other catalogs that support namespace properties.

  • Creates a base set of catalog tests that check namespace behavior
  • Consolidates JDBC exception handling by combining logic in fetch, exists, and execute helper methods
  • Fixes namespace listing that needs to combine namespaces that exist from properties or tables

@github-actions github-actions Bot added the core label Feb 25, 2022
return execute(err -> {}, sql, args);
}

private int execute(Consumer<SQLException> sqlErrorHandler, String sql, String... args) {

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.

Error handler seems unnecessary since it's never used and throws anyway (as opposed to delegating to the handler).

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.

This is used in one case: https://github.com/apache/iceberg/pull/4231/files#diff-a5f7966c7493d463facbf7e76fe0fa3db1b637d29ee12e5b3628a5e5dd289e0aR195

I did this to avoid having a SQLException try/catch in every method.

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.

Makes sense. Missed that usage.

Comment thread core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated
@rdblue rdblue merged commit f0bacc8 into apache:master Feb 25, 2022
@rdblue

rdblue commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @danielcweeks!

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.

2 participants