Force table / view location #8516

adutra · 2024-05-14T14:58:31Z

No description provided.

...e/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1GenericResource.java

dimas-b · 2024-05-14T15:32:25Z

...e/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1GenericResource.java

+                    "Invalid create requirements: %s",
+                    invalidRequirements);
+                String location = defaultTableLocation(decoded.warehouse(), key);
+                builder.addUpdate(setLocation(location));


What if AssertCreate is missing? Aren't we losing the SetLocation update in that case?

Yes we are. That's the idea: ignore any locations sent by the client, and use our own.

I mean should we not do builder.addUpdate(setLocation(defaultLocation)) even if/when AssertCreate is not present?

I think not, because in this case it's an update, so the table already exists at its default location. Am I missing something?

To clarify: AssertCreate is a requirement that the client chooses to submit. Can we be sure that a table cannot be created without submitting an AssertCreate?

While it seems that it is not possible to create a table without an AssertCreate now, it seems to me that it's a co-incidence rather than a deliberate check. From the REST API perspective an AssertCreate does not have to be present.

Should we set the catalog-managed location in CatalogServiceImpl perhaps rather than at the REST level?

Yes I was leaning towards that already, and you convinced me. I will revisit this PR entirely.

...vice/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1ResourceBase.java

adutra · 2024-05-15T18:51:30Z

I tried to push the location computation down to CatalogServiceImpl, however many tests are now failing, and I still have no clue why. Will investigate more tomorrow.

snazy · 2024-05-15T19:47:51Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

+    String baseLocation = warehouseLocation;
+    Namespace ns = key.getNamespace();
+    if (!ns.isEmpty()) {
+      baseLocation = concatLocation(warehouseLocation, ns.toString());


I think using the key elements are probably easier to handle - would also prevent the namespace check/handling.

snazy · 2024-05-15T19:49:54Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

@@ -755,7 +757,33 @@ public static long safeUnbox(Long value, long defaultValue) {
    return value != null ? value : defaultValue;
  }

-  public static NessieTableSnapshot newIcebergTableSnapshot(List<IcebergMetadataUpdate> updates) {
+  public static String defaultIcebergLocation(String warehouseLocation, ContentKey key) {
+    String baseLocation = warehouseLocation;


I think, we should respect the "object store naming strategy" here - or maybe apply it unconditionally. @dimas-b wdyt?

I think it makes sense. Also, @adutra and I agreed (other comment thread) to tackle actual location construction in a follow-up PR :)

If you prefer to do this here, fine with me. You tell me :-)

Maybe flat structure (no namespace) in this PR, plus more advanced strategies as a follow-up?

What do you mean by "object store naming strategy", is it whether to use path-style or virtual-host style?

Nah - with "naming strategy" I meant the "randomized prefix" from o.a.iLocationProviders.ObjectStoreLocationProvider - but it seems my brain was somewhere else, because this change is about the base location - the the actual object location. Please disregard my comment and ignore everything I said or recommended above.

snazy · 2024-05-15T19:50:30Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

+    // TODO: support TableProperties.WRITE_METADATA_LOCATION
+    String location =
+        String.format(
+            "%s_%s/metadata/00000-%s.metadata.json", baseLocation, randomUUID(), randomUUID());


Isn't the 1st UUID the table's ("fixed") UUID?

Yes, I was mixing base location and metadata json location. It's fixed now.

snazy · 2024-05-15T19:52:08Z

catalog/model/src/main/java/org/projectnessie/catalog/model/ops/CatalogOperation.java

+   *
+   * <p>If not set, the default warehouse will be used.
+   */
+  String warehouse();


Should this be Optional<String>?

That was my first impl, but in fact there is no need of that because of how CatalogConfig.getWarehouse is implemented: it already handles nulls gracefully. I should have added @Nullable here though, I will fix that.

snazy · 2024-05-15T19:53:17Z

...og/service/impl/src/main/java/org/projectnessie/catalog/service/impl/CatalogServiceImpl.java

@@ -505,16 +494,8 @@ private CompletionStage<MultiTableUpdate> applyIcebergTableCommitOperation(
                })
            .thenApply(
                nessieSnapshot -> {
-                  // TODO: support GZIP
-                  // TODO: support TableProperties.WRITE_METADATA_LOCATION


Note: We want ignore this property, yes.

snazy · 2024-05-15T19:58:37Z

...e/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1GenericResource.java

@@ -138,14 +145,33 @@ public Uni<Void> commitTransaction(
    commitTransactionRequest.tableChanges().stream()
        .map(
            tableChange -> {
+              List<IcebergMetadataUpdate> updates =
+                  tableChange.updates().stream()
+                      .filter(update -> !(update instanceof SetLocation))


Wonder if we should make the SetLocation update-operation a noop?

snazy · 2024-05-15T20:03:35Z

...e/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1GenericResource.java

+                    tableChange.requirements().stream()
+                        .filter(req -> !(req instanceof AssertCreate))
+                        .collect(Collectors.toList());
+                checkArgument(


Don't we already do this implicitly via the tableExists/viewExists parameter in IcebergUpdateRequirement?

The goal here is to make sure that, if assert-create is present, then no other requirement is present. I don't see this being done in IcebergUpdateRequirement.

Ah, right. But wouldn't that not work with if (requirements.size()!=1) ?

dimas-b · 2024-05-16T18:08:25Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

+    // To avoid sharing same table path between two tables with same name, use uuid in the table
+    // path.
+    // Also: we deliberately ignore the TableProperties.WRITE_METADATA_LOCATION property here.
+    // TODO: support GZIP ??


nit: How is GZIP related to base locations?

I just moved the TODO form its old location to here. I can remove it.

dimas-b · 2024-05-16T18:21:17Z

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

+    if (!ns.isEmpty()) {
+      baseLocation = concatLocation(warehouseLocation, ns.toString());
+    }
+    baseLocation = concatLocation(baseLocation, key.getName());


Can we go with a flat namespace until we resolve #8524?

Sorry... what is a flat namespace?

I meant no ns in the path, but I guess key.getName() can also contain special chars, so I'm ok leaving this method "as is" for now and addressing later, under #8524

...eberg/src/main/java/org/projectnessie/catalog/formats/iceberg/nessie/NessieModelIceberg.java

The merge-base changed after approval.

dimas-b reviewed May 14, 2024

View reviewed changes

...vice/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1ResourceBase.java Outdated Show resolved Hide resolved

dimas-b requested changes May 14, 2024

View reviewed changes

...vice/rest/src/main/java/org/projectnessie/catalog/service/rest/IcebergApiV1ResourceBase.java Outdated Show resolved Hide resolved

snazy force-pushed the feature/nessie-catalog-server branch from 939f4c8 to a710182 Compare May 15, 2024 10:23

dimas-b self-requested a review May 15, 2024 15:26

adutra force-pushed the table-location-oss branch from 84fc21b to 7df762b Compare May 15, 2024 18:50

snazy reviewed May 15, 2024

View reviewed changes

dimas-b mentioned this pull request May 16, 2024

Do not allow special characters in base table locations #8524

Open

dimas-b previously approved these changes May 16, 2024

View reviewed changes

snazy force-pushed the feature/nessie-catalog-server branch from b8c6df1 to b1b16ff Compare May 17, 2024 07:57

adutra added 7 commits May 17, 2024 10:52

Force table / view location

47dcdbb

review

5ea9bd9

fix base location

de17bcd

refactor hasAssertCreate

31116a3

stupid mistake

13d0d78

stupid mistake

165976e

remove todo

0bb356d

adutra force-pushed the table-location-oss branch from 0922ac2 to 0bb356d Compare May 17, 2024 08:52

remove ns from base location

4f03fe7

dimas-b previously approved these changes May 17, 2024

View reviewed changes

adutra enabled auto-merge (squash) May 17, 2024 15:05

put back ns in location

be92c8d

adutra dismissed dimas-b’s stale review via be92c8d May 17, 2024 15:29

dimas-b approved these changes May 17, 2024

View reviewed changes

adutra merged commit 35af148 into projectnessie:feature/nessie-catalog-server May 17, 2024
16 checks passed

snazy pushed a commit that referenced this pull request May 24, 2024

Iceberg REST: force table / view location (#8516)

a2cf9dc

snazy pushed a commit that referenced this pull request May 25, 2024

Iceberg REST: force table / view location (#8516)

42e0fd4

snazy pushed a commit that referenced this pull request May 25, 2024

Iceberg REST: force table / view location (#8516)

b116189

snazy pushed a commit that referenced this pull request May 27, 2024

Iceberg REST: force table / view location (#8516)

7a7ce0e

snazy pushed a commit that referenced this pull request May 27, 2024

Iceberg REST: force table / view location (#8516)

9330f7e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force table / view location #8516

Force table / view location #8516

adutra commented May 14, 2024

dimas-b May 14, 2024

adutra May 14, 2024

dimas-b May 14, 2024

adutra May 14, 2024

dimas-b May 14, 2024

dimas-b May 14, 2024 •

edited

adutra May 15, 2024

adutra commented May 15, 2024

snazy May 15, 2024

snazy May 15, 2024

dimas-b May 16, 2024

adutra May 16, 2024

dimas-b May 16, 2024

adutra May 16, 2024

snazy May 16, 2024

snazy May 15, 2024

adutra May 16, 2024

snazy May 15, 2024

adutra May 16, 2024

snazy May 16, 2024

snazy May 15, 2024

snazy May 15, 2024

snazy May 15, 2024

adutra May 16, 2024

snazy May 16, 2024

dimas-b May 16, 2024

adutra May 17, 2024

dimas-b May 16, 2024

adutra May 17, 2024

dimas-b May 17, 2024

Force table / view location #8516

Force table / view location #8516

Conversation

adutra commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b May 14, 2024 •

edited