snazy commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1680549331
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -71,6 +71,18 @@ public void afterEach() throws Exception {
anotherCatalog.close();
}
+ @Test
+ public void testCreateNamespacesWithLevels() {
+ catalog.createNamespace(Namespace.of("l1", "l2", "l3", "l4"));
+
assertThat(catalog.listNamespaces()).containsExactlyInAnyOrder(Namespace.of("l1"));
Review Comment:
Why `InAnyOrder` ?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieIcebergClient.java:
##########
@@ -132,13 +132,33 @@ public void testCreateNamespace() throws
NessieConflictException, NessieNotFound
.extracting(LogResponse.LogEntry::getCommitMeta)
.extracting(CommitMeta::getMessage, CommitMeta::getAuthor,
CommitMeta::getProperties)
.containsExactly(
- "create namespace a",
+ "create namespace [a]",
"iceberg-user",
ImmutableMap.of(
"application-type", "iceberg",
"app-id", "iceberg-nessie"));
}
+ @Test
+ public void testCreateNamespaceWithMultipleLevels()
+ throws NessieConflictException, NessieNotFoundException {
+ String branch = "createNamespaceWithMultipleLevelsBranch";
+ createBranch(branch);
+ Map<String, String> catalogOptions =
+ Map.of(
+ CatalogProperties.USER, "iceberg-user",
+ CatalogProperties.APP_ID, "iceberg-nessie");
+ NessieIcebergClient client = new NessieIcebergClient(api, branch, null,
catalogOptions);
+
+ client.createNamespace(Namespace.of("a", "b", "c"), Map.of());
+ assertThat(client.listNamespaces(Namespace.of("a"))).isNotNull();
Review Comment:
This should assert on the result - not just "not null"
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -219,15 +221,41 @@ public void createNamespace(Namespace namespace,
Map<String, String> metadata) {
checkNamespaceIsValid(namespace);
getRef().checkMutable();
ContentKey key = ContentKey.of(namespace.levels());
- org.projectnessie.model.Namespace content =
- org.projectnessie.model.Namespace.of(key.getElements(), metadata);
try {
Content existing =
api.getContent().reference(getReference()).key(key).get().get(key);
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = Lists.newArrayList();
+ Map<ContentKey, Content> contentInfo = Maps.newHashMap();
+ for (int i = 0; i < namespace.levels().length; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i +
1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content parentContent =
+
api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
Review Comment:
It's better to do only one call to Nessie for all required content-keys -
including the one in line 225.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -219,15 +221,41 @@ public void createNamespace(Namespace namespace,
Map<String, String> metadata) {
checkNamespaceIsValid(namespace);
getRef().checkMutable();
ContentKey key = ContentKey.of(namespace.levels());
- org.projectnessie.model.Namespace content =
- org.projectnessie.model.Namespace.of(key.getElements(), metadata);
try {
Content existing =
api.getContent().reference(getReference()).key(key).get().get(key);
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = Lists.newArrayList();
+ Map<ContentKey, Content> contentInfo = Maps.newHashMap();
+ for (int i = 0; i < namespace.levels().length; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i +
1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content parentContent =
+
api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, parentContent);
+ }
+
+ List<Operation.Put> putOperations = Lists.newArrayList();
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry :
contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace content;
+ if (contentKeyContentEntry.getKey().equals(key)) {
+ content = org.projectnessie.model.Namespace.of(key.getElements(),
metadata);
+ } else {
+ content =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(),
ImmutableMap.of());
+ }
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(),
content));
+ }
+ }
+
try {
- commitRetry("create namespace " + key, Operation.Put.of(key, content));
+ commitRetry("create namespace " + keys, putOperations.toArray(new
Operation.Put[0]));
Review Comment:
The commit message would say that it created all parent namespaces,
including those that already existed.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -71,6 +71,18 @@ public void afterEach() throws Exception {
anotherCatalog.close();
}
+ @Test
+ public void testCreateNamespacesWithLevels() {
Review Comment:
Not sure how this test uses multiple clients (`TestMultipleClients`) and the
difference to the test in `TestNessieIcebergClient`.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -219,15 +221,41 @@ public void createNamespace(Namespace namespace,
Map<String, String> metadata) {
checkNamespaceIsValid(namespace);
getRef().checkMutable();
ContentKey key = ContentKey.of(namespace.levels());
- org.projectnessie.model.Namespace content =
- org.projectnessie.model.Namespace.of(key.getElements(), metadata);
try {
Content existing =
api.getContent().reference(getReference()).key(key).get().get(key);
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = Lists.newArrayList();
+ Map<ContentKey, Content> contentInfo = Maps.newHashMap();
+ for (int i = 0; i < namespace.levels().length; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i +
1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content parentContent =
+
api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, parentContent);
+ }
+
+ List<Operation.Put> putOperations = Lists.newArrayList();
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry :
contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace content;
+ if (contentKeyContentEntry.getKey().equals(key)) {
Review Comment:
Nit: I'd prefer a ternary for the namespace properties.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieIcebergClient.java:
##########
@@ -132,13 +132,33 @@ public void testCreateNamespace() throws
NessieConflictException, NessieNotFound
.extracting(LogResponse.LogEntry::getCommitMeta)
.extracting(CommitMeta::getMessage, CommitMeta::getAuthor,
CommitMeta::getProperties)
.containsExactly(
- "create namespace a",
+ "create namespace [a]",
"iceberg-user",
ImmutableMap.of(
"application-type", "iceberg",
"app-id", "iceberg-nessie"));
}
+ @Test
+ public void testCreateNamespaceWithMultipleLevels()
+ throws NessieConflictException, NessieNotFoundException {
+ String branch = "createNamespaceWithMultipleLevelsBranch";
+ createBranch(branch);
+ Map<String, String> catalogOptions =
Review Comment:
What's the reason of this map for this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
