Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
bryanck merged PR #10186: URL: https://github.com/apache/iceberg/pull/10186 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
bryanck commented on PR #10186: URL: https://github.com/apache/iceberg/pull/10186#issuecomment-2113284956 Thanks for the contribution @ajantha-bhat , it looks good! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on PR #10186: URL: https://github.com/apache/iceberg/pull/10186#issuecomment-2111667325 ping @bryanck -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1595087812
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -47,7 +54,7 @@ public class IcebergWriterFactoryTest {
@ValueSource(booleans = {true, false})
@SuppressWarnings("unchecked")
public void testAutoCreateTable(boolean partitioned) {
-Catalog catalog = mock(Catalog.class);
+Catalog catalog = mock(InMemoryCatalog.class);
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1595087423
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -83,4 +90,26 @@ public void testAutoCreateTable(boolean partitioned) {
assertThat(specCaptor.getValue().isPartitioned()).isEqualTo(partitioned);
assertThat(propsCaptor.getValue()).containsKey("test-prop");
}
+
+ @Test
+ public void testNamespaceCreation() throws IOException {
+TableIdentifier tableIdentifier =
+TableIdentifier.of(Namespace.of("foo1", "foo2", "foo3"), "bar");
+Schema schema = new Schema(Types.NestedField.required(1, "id",
Types.StringType.get()));
+
+try (InMemoryCatalog catalog = new InMemoryCatalog()) {
Review Comment:
done. Updated the existing testcase
##
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/IcebergWriterFactory.java:
##
@@ -112,4 +118,18 @@ Table autoCreateTable(String tableName, SinkRecord sample)
{
});
return result.get();
}
+
+ @VisibleForTesting
+ static void createNamespaceIfNotExist(Catalog catalog, Namespace
identifierNamespace) {
+String[] levels = identifierNamespace.levels();
+for (int index = 0; index < levels.length; index++) {
+ Namespace namespace = Namespace.of(Arrays.copyOfRange(levels, 0, index +
1));
+ try {
+((SupportsNamespaces) catalog).createNamespace(namespace);
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
bryanck commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1579467601
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -83,4 +90,26 @@ public void testAutoCreateTable(boolean partitioned) {
assertThat(specCaptor.getValue().isPartitioned()).isEqualTo(partitioned);
assertThat(propsCaptor.getValue()).containsKey("test-prop");
}
+
+ @Test
+ public void testNamespaceCreation() throws IOException {
+TableIdentifier tableIdentifier =
+TableIdentifier.of(Namespace.of("foo1", "foo2", "foo3"), "bar");
+Schema schema = new Schema(Types.NestedField.required(1, "id",
Types.StringType.get()));
+
+try (InMemoryCatalog catalog = new InMemoryCatalog()) {
Review Comment:
I'd prefer using the ArgumentCaptor approach, rather than relying on
InMemoryCatalog. This is consistent with the auto create test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1575543923
##
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/IcebergWriterFactory.java:
##
@@ -112,4 +117,18 @@ Table autoCreateTable(String tableName, SinkRecord sample)
{
});
return result.get();
}
+
+ @VisibleForTesting
+ static void createNamespaceIfNotExist(Catalog catalog, Namespace
identifierNamespace) {
+String[] levels = identifierNamespace.levels();
+for (int index = 0; index < levels.length; index++) {
+ Namespace namespace = Namespace.of(Arrays.copyOfRange(levels, 0, index +
1));
+ try {
+((SupportsNamespaces) catalog).createNamespace(namespace);
+ } catch (AlreadyExistsException ex) {
Review Comment:
I assumed if the auto create table is enabled, means user has permission to
create tables. So, They can create Namespaces also. But that assumptions can be
wrong for some catalogs. Let me catch `ForbiddenException` also for ignoring it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
Fokko commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1572639039
##
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/IcebergWriterFactory.java:
##
@@ -112,4 +117,18 @@ Table autoCreateTable(String tableName, SinkRecord sample)
{
});
return result.get();
}
+
+ @VisibleForTesting
+ static void createNamespaceIfNotExist(Catalog catalog, Namespace
identifierNamespace) {
+String[] levels = identifierNamespace.levels();
+for (int index = 0; index < levels.length; index++) {
+ Namespace namespace = Namespace.of(Arrays.copyOfRange(levels, 0, index +
1));
+ try {
+((SupportsNamespaces) catalog).createNamespace(namespace);
+ } catch (AlreadyExistsException ex) {
Review Comment:
We went the other way with Flink: https://github.com/apache/iceberg/pull/7795
Can we also swallow the exception where the user doesn't have permission to
create a namespace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1572380923
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -83,4 +90,26 @@ public void testAutoCreateTable(boolean partitioned) {
assertThat(specCaptor.getValue().isPartitioned()).isEqualTo(partitioned);
assertThat(propsCaptor.getValue()).containsKey("test-prop");
}
+
+ @Test
+ public void testNamespaceCreation() throws IOException {
Review Comment:
This is unit test. In future when the integration tests are added (currently
missing for the kafka connect), we can add the auto table creation with the
missing namespaces integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1572380923
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -83,4 +90,26 @@ public void testAutoCreateTable(boolean partitioned) {
assertThat(specCaptor.getValue().isPartitioned()).isEqualTo(partitioned);
assertThat(propsCaptor.getValue()).containsKey("test-prop");
}
+
+ @Test
+ public void testNamespaceCreation() throws IOException {
Review Comment:
This is unit test. In future when the integration tests are added, we can
add the auto table creation with the missing namespaces integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Kafka-connect: Handle namespace creation for auto table creation [iceberg]
ajantha-bhat commented on code in PR #10186:
URL: https://github.com/apache/iceberg/pull/10186#discussion_r1572379348
##
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java:
##
@@ -47,7 +54,7 @@ public class IcebergWriterFactoryTest {
@ValueSource(booleans = {true, false})
@SuppressWarnings("unchecked")
public void testAutoCreateTable(boolean partitioned) {
-Catalog catalog = mock(Catalog.class);
+Catalog catalog = mock(InMemoryCatalog.class);
Review Comment:
Need to change it to some catalog that supports namespace creation. As the
code now calls `createNamespaceIfNotExist`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
