-
Notifications
You must be signed in to change notification settings - Fork 3.4k
BigQuery: Add table validity check for BigQueryMetastoreCatalog #14113
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
Changes from 2 commits
bef7569
b050563
9a2e653
acddd8f
8a33121
c9979c1
16a835a
056da20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,11 +21,19 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_BIGQUERY; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.apache.iceberg.gcp.bigquery.BigQueryMetastoreCatalog.PROJECT_ID; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.assertj.core.api.Assertions.assertThat; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.ArgumentMatchers.any; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.spy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import static org.mockito.Mockito.when; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.io.File; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.CatalogProperties; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.MetadataTableUtils; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.Table; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.TableMetadata; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.TableOperations; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
nastra marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.catalog.CatalogTests; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.catalog.Namespace; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.apache.iceberg.catalog.TableIdentifier; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -39,6 +47,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.jupiter.api.io.TempDir; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.jupiter.params.ParameterizedTest; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.junit.jupiter.params.provider.ValueSource; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.mockito.MockedStatic; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.mockito.Mockito; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
nastra marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class TestBigQueryCatalog extends CatalogTests<BigQueryMetastoreCatalog> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| @TempDir private File tempFolder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -171,4 +181,85 @@ public void renameTableNamespaceMissing() { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testRenameTableMissingSourceTable() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| super.testRenameTableMissingSourceTable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testIsValidIdentifierWithValidSingleLevelNamespace() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier validIdentifier = TableIdentifier.of("dataset1", "table1"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertThat(catalog.isValidIdentifier(validIdentifier)).isTrue(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testIsValidIdentifierWithInvalidMultiLevelNamespace() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier invalidIdentifier = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: all of these can be inlined
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, formatting creates multiple lines for a few of the more verbose cases. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier.of(Namespace.of("level1", "level2"), "table1"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertThat(catalog.isValidIdentifier(invalidIdentifier)).isFalse(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testIsValidIdentifierWithThreeLevelNamespace() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier invalidIdentifier = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier.of(Namespace.of("level1", "level2", "level3"), "table1"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertThat(catalog.isValidIdentifier(invalidIdentifier)).isFalse(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testIsValidIdentifierWithEmptyNamespace() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier invalidIdentifier = TableIdentifier.of(Namespace.empty(), "table1"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertThat(catalog.isValidIdentifier(invalidIdentifier)).isFalse(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void testLoadMetadataTableIsCalled() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I follow why this is being tested here, can you elaborate please?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to test that loading a metadata table will actually happen with the fix in place, since that's technically a valid case of specifying an invalid table name.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am bit confused too, if the goal is that that the metadata table loading works correctly post this fix, shouldnt we enable this test instead ? iceberg/bigquery/src/test/java/org/apache/iceberg/gcp/bigquery/TestBigQueryCatalog.java Lines 147 to 149 in a73a5c0
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will move the test case there!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason why can't re-use the same test ? as if just enable this test from the base class it test if we can read the Files metadata table ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I agree, we should be able to remove this test method entirely and rely on the testing behavior defined in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch, heres the test with the same name in CatalogTests.java iceberg/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Lines 929 to 950 in a73a5c0
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good thank you. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create a spy of the catalog to verify method calls | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| BigQueryMetastoreCatalog spyCatalog = spy(catalog); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create mock objects | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableOperations mockOps = Mockito.mock(TableOperations.class); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableMetadata mockMetadata = Mockito.mock(TableMetadata.class); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Table mockMetadataTable = Mockito.mock(Table.class); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Mock the table operations to return metadata (indicating base table exists) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(mockOps.current()).thenReturn(mockMetadata); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create the expected base table identifier that will be extracted from the metadata table | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // identifier | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier expectedBaseTableId = TableIdentifier.of("dataset1", "table1"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| when(spyCatalog.newTableOps(expectedBaseTableId)).thenReturn(mockOps); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use MockedStatic to mock the static MetadataTableUtils.createMetadataTableInstance call | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try (MockedStatic<MetadataTableUtils> mockedUtils = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Mockito.mockStatic(MetadataTableUtils.class)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| mockedUtils | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| .when( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| () -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| MetadataTableUtils.createMetadataTableInstance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableOperations.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(String.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableIdentifier.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableIdentifier.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| .thenReturn(mockMetadataTable); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create a metadata table identifier | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier metadataTableId = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| TableIdentifier.of(Namespace.of("dataset1", "table1"), "partitions"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Call loadTable which should trigger the metadata table loading path | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Table result = spyCatalog.loadTable(metadataTableId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify that MetadataTableUtils.createMetadataTableInstance was called | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This confirms that loadMetadataTable path was taken | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| mockedUtils.verify( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| () -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| MetadataTableUtils.createMetadataTableInstance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableOperations.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(String.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableIdentifier.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any(TableIdentifier.class), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| any())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify the result is the mocked metadata table | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assertThat(result).isSameAs(mockMetadataTable); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.