You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zymap (via GitHub)" <gi...@apache.org> on 2024/07/04 10:52:17 UTC
[PR] Support create multiple element ns together for nessie [iceberg]
zymap opened a new pull request, #10630:
URL: https://github.com/apache/iceberg/pull/10630
---
### Motivation
Create multiple level namespace together when creating namespace. For more time, the namespace level is one. But sometimes when we have multiple levels to construct the namespace, the Nessie will throw the parent namespace is not found.
For other catalogs, they may merge the multiple levels to one level so they don’t have such an issue.
We use pulsar build on the iceberg, so map the pulsar tenant/namespace to the namespace name. There are two levels then failed to create the table.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668419668
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -226,8 +229,32 @@ public void createNamespace(Namespace namespace, Map<String, String> metadata) {
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = new ArrayList<>(namespace.levels().length);
+ Map<ContentKey, Content> contentInfo = new HashMap<>();
+ for (int i = 0; i < namespace.levels().length - 1; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i + 1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content c = api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, c);
+ }
+
+
+ ArrayList<Operation.Put> putOperations = new ArrayList<>(keys.size());
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry : contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace parentContent =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(), ImmutableMap.of());
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(), parentContent));
Review Comment:
If we auto create the missing parent namespaces during the current namespace, who will be responsible for dropping the parent namespace when we drop the child namespace?
So, we need to handle drop namespace as well.
But what if we the user current user don't have permission to drop the parent 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668448186
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -226,8 +229,32 @@ public void createNamespace(Namespace namespace, Map<String, String> metadata) {
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = new ArrayList<>(namespace.levels().length);
+ Map<ContentKey, Content> contentInfo = new HashMap<>();
+ for (int i = 0; i < namespace.levels().length - 1; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i + 1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content c = api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, c);
+ }
+
+
+ ArrayList<Operation.Put> putOperations = new ArrayList<>(keys.size());
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry : contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace parentContent =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(), ImmutableMap.of());
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(), parentContent));
Review Comment:
> So, we need to handle drop namespace as well.
No
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1666899521
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -290,6 +291,14 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
@Override
public void createNamespace(Namespace namespace, Map<String, String> metadata) {
+ if (namespace.levels().length > 1) {
Review Comment:
@zymap we explicitly removed that functionality from the server side a while ago.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2216704059
ACK and agree that it is a grey area in the spec. Maybe good to discuss in iceberg mailing list about standardising it and updating the spec.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668253804
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -290,6 +291,14 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
@Override
public void createNamespace(Namespace namespace, Map<String, String> metadata) {
+ if (namespace.levels().length > 1) {
Review Comment:
@snazy I move the impl to the NessieIcebergClient. PTAL.
>we explicitly removed that functionality from the server side a while ago.
You mean the Nessie namespace? Could you please point me to the PR?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2477675538
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10630: Support create multiple element ns together for nessie
URL: https://github.com/apache/iceberg/pull/10630
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668419668
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -226,8 +229,32 @@ public void createNamespace(Namespace namespace, Map<String, String> metadata) {
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = new ArrayList<>(namespace.levels().length);
+ Map<ContentKey, Content> contentInfo = new HashMap<>();
+ for (int i = 0; i < namespace.levels().length - 1; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i + 1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content c = api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, c);
+ }
+
+
+ ArrayList<Operation.Put> putOperations = new ArrayList<>(keys.size());
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry : contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace parentContent =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(), ImmutableMap.of());
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(), parentContent));
Review Comment:
If we auto create the missing parent namespaces during the current namespace, who will be responsible for dropping these auto created parent namespace when we drop the child namespace?
So, we need to handle drop namespace as well.
But what if we the user current user don't have permission to drop the parent 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2463470526
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2222138728
Lets wait for mailing list conclusion. We can send a reminder on mailing list if no response by Monday.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668398248
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -290,6 +291,14 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
@Override
public void createNamespace(Namespace namespace, Map<String, String> metadata) {
+ if (namespace.levels().length > 1) {
Review Comment:
We don't remove namespaces - we have removed the explicit namespace endpoints in Nessie's REST API
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2208681983
@snazy Please take a look when you have time. Thanks! :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1666167004
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -290,6 +291,14 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
@Override
public void createNamespace(Namespace namespace, Map<String, String> metadata) {
+ if (namespace.levels().length > 1) {
Review Comment:
@snazy Do you think it makes sense to move this create action to the server side? We also met the same issue when using the REST way to request Nessie.
I am not cleared with the namespace in the Nessie, not sure if it is a normal request.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2232577028
@ajantha-bhat @snazy Seems the namespace behaviors will not scandalize. So what do you think about this PR? Would it make sense to Nessie to create a namespace recursively? Then let the RBAC to control if user has the permission to create the parent namespace. Or you think it should be done on the user side when they choose Nessie as the catalog?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2244078738
Thanks for the review! I will address the comments later
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2216082740
> Considering the potential access control tie up around namespaces, I wouldn't recommend auto creating missing namespaces. The current user may not have permission to drop parent namespaces and it may need manual clean up again from admin.
>
> It is not very hard to write a script to create namespaces from admin level instead of code level. So, I am not really sure about this change.
>
> Also, is there any catalog doing this already? Can you please point me to it?
@ajantha-bhat Our case is in the message queue and makes the data sink to the iceberg. We have a lot of namespace so it is not easy to do this with a script.
Yes, it would have permission issue but I think that should be properly configured by the user. If you want it auto-created, you can give a proper role to do this. That should be a part of the RBAC.
As @snazy said, the functionality is not very clear in the iceberg. The namespace levels is different for the different catalogs. I did some tests, for JDBC, and it will create all the levels. But for others, they may treat it as a single string.
For deletions, my understanding is it should never done automatically. You never know what is used for and who uses it. It must be a manual operation because you should be responsible for checking nobody still using 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#issuecomment-2221807269
@ajantha-bhat @snazy I think we can have this as a temporary solution. At least users don't need to care about the difference when using the createNamespace. WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1665606754
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -290,6 +291,14 @@ public void renameTable(TableIdentifier from, TableIdentifier to) {
@Override
public void createNamespace(Namespace namespace, Map<String, String> metadata) {
+ if (namespace.levels().length > 1) {
Review Comment:
This would fail, if a parent namespace already exists.
A better place to do this would be `org.apache.iceberg.nessie.NessieIcebergClient#createNamespace`:
* load all required namespaces
* add the `Put` operation for all non-existing parent namespaces to the commit operation
OTOH I'm not sure about the actual contract of this function. Some implementation seem to allow creating a namespaces when parent namespaces do not exist - so the contract isn't clear, aka the functionality per se seems okay to me.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Support create multiple element ns together for nessie [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #10630:
URL: https://github.com/apache/iceberg/pull/10630#discussion_r1668419668
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -226,8 +229,32 @@ public void createNamespace(Namespace namespace, Map<String, String> metadata) {
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = new ArrayList<>(namespace.levels().length);
+ Map<ContentKey, Content> contentInfo = new HashMap<>();
+ for (int i = 0; i < namespace.levels().length - 1; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i + 1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content c = api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, c);
+ }
+
+
+ ArrayList<Operation.Put> putOperations = new ArrayList<>(keys.size());
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry : contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace parentContent =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(), ImmutableMap.of());
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(), parentContent));
Review Comment:
If we auto create the missing parent namespaces during the current namespace, who will be responsible for dropping these auto created parent namespace when we drop the child namespace?
So, we need to handle drop namespace as well.
But what if we the current user don't have permission to drop the parent namespace?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -226,8 +229,32 @@ public void createNamespace(Namespace namespace, Map<String, String> metadata) {
if (existing != null) {
throw namespaceAlreadyExists(key, existing, null);
}
+
+ // check if the parent namespace exists
+ List<ContentKey> keys = new ArrayList<>(namespace.levels().length);
+ Map<ContentKey, Content> contentInfo = new HashMap<>();
+ for (int i = 0; i < namespace.levels().length - 1; i++) {
+ Namespace parent = Namespace.of(Arrays.copyOf(namespace.levels(), i + 1));
+ ContentKey parentKey = ContentKey.of(parent.levels());
+ keys.add(parentKey);
+ Content c = api.getContent().reference(getReference()).key(parentKey).get().get(parentKey);
+ contentInfo.put(parentKey, c);
+ }
+
+
+ ArrayList<Operation.Put> putOperations = new ArrayList<>(keys.size());
+ for (Map.Entry<ContentKey, Content> contentKeyContentEntry : contentInfo.entrySet()) {
+ if (contentKeyContentEntry.getValue() == null) {
+ org.projectnessie.model.Namespace parentContent =
+ org.projectnessie.model.Namespace.of(
+ contentKeyContentEntry.getKey().getElements(), ImmutableMap.of());
+ putOperations.add(Operation.Put.of(contentKeyContentEntry.getKey(), parentContent));
Review Comment:
If we auto create the missing parent namespaces during the current namespace, who will be responsible for dropping these auto created parent namespace when we drop the child namespace?
So, we need to handle drop namespace as well.
But what if we the current user don't have permission to drop/create the parent 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org