Core: Assign fresh IDs to view schema #10253

nastra · 2024-04-30T10:26:41Z

This addresses #9596 (comment) and assigns fresh IDs to a View's schema when creating/replacing a view

nastra · 2024-04-30T13:38:34Z

core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java

  private static final Schema OTHER_SCHEMA =
-      new Schema(7, required(1, "some_id", Types.IntegerType.get()));
+      new Schema(


changing this to show that the new column get's a re-assigned column ID

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

rdblue

The main issue is that the highest ID isn't correct. There should also be a test for a table with an old schema that contains a higher ID than the current schema to show it is not reassigned.

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java

github-actions · 2024-11-02T00:15:05Z

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 [email protected] list. Thank you for your contributions.

amogh-jahagirdar

Looks good to me (had a question that's worth double checking but I'm pretty sure there's no issue), thanks @nastra!

amogh-jahagirdar · 2024-12-09T00:08:18Z

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java

+                    Types.NestedField.optional(1, "new_id", Types.IntegerType.get(), "some ID"),
+                    Types.NestedField.optional(2, "new_data", Types.StringType.get(), "some data"))


So the first field ID changed to 1 because highestFieldId is set to 0 and then incremented when assigning the new IDs? This should be fine and aligns with whatever we do for tables. It also does not impact existing views because we're assigning the fresh IDs from a higher field ID

yes correct, that's exactly how it is done

rdblue · 2024-12-09T17:18:57Z

core/src/main/java/org/apache/iceberg/view/ViewUtil.java


 public class ViewUtil {
  private ViewUtil() {}

  public static String fullViewName(String catalog, TableIdentifier ident) {
    return catalog + "." + ident;
  }
+
+  public static Schema assignFreshIds(ViewMetadata base, Schema newSchema) {


I think that the current arguments to this method pass too much state. Utility methods should require a minimal amount of information so it is clear what they depend on when called. Rather than passing the entire ViewMetadata, this method should accept newSchema, the base schema, and the highest field ID.

I'm also not sure that it makes sense to have a util method that just calls another util method, TypeUtil.assignFreshIds. I think once you apply the principle of passing minimal state, you basically end up with a wrapper around the TypeUtil method that creates an atomic integer.

rdblue · 2024-12-09T17:19:43Z

core/src/main/java/org/apache/iceberg/view/ViewUtil.java

+        new AtomicInteger(highestFieldId(base.schemas()))::incrementAndGet);
+  }
+
+  public static int maxVersionId(ViewMetadata metadata) {


This method has the same issue. Instead of passing ViewMetadata, it should accept Collection<ViewVersion>.

I've removed this refactoring around maxVersionId for now since assigning fresh ids moved into ViewMetadata and it didn't seem appropriate anymore to include this refactoring (I'll open a separate PR for this)

rdblue · 2024-12-09T17:46:21Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

@@ -1341,6 +1343,8 @@ public View create() {
      Preconditions.checkState(
          null != defaultNamespace, "Cannot create view without specifying a default namespace");

+      this.schema = TypeUtil.assignFreshIds(schema, new AtomicInteger(0)::incrementAndGet);


It looks like this is being done in the wrong place because we need to assign fresh IDs in both the REST catalog here and in BaseMetastoreViewCatalog. We don't want to create a situation where implementations need to know to assign fresh IDs.

For tables, the initial assignment is done in TableMetadata.newTableMetadata and the reassignment for replace happens in TableMetadata.buildReplacement. Views should have a similar approach, where creating a replacement ViewMetadata automatically handles the reassignment so callers don't need to know and do it manually.

yes you're right. One of the issues was that there wasn't a straightforward way inside ViewMetadata of detecting whether ViewMetadata is created the first time or replaced. I think we can actually use the history field (or alternatively introduce a new field) to detect whether the view is created/replaced. I've updated the code so that no additional changes outside of ViewMetadata are required (and also moved the tests now into TestViewMetadata

rdblue · 2024-12-09T17:47:59Z

core/src/test/java/org/apache/iceberg/view/TestViewUtil.java

+                schemaOne)
+            .build();
+
+    newSchema = ViewUtil.assignFreshIds(metadata, newSchema);


This test demonstrates the problem that I pointed out above. It is necessary to construct a fake ViewMetadata to test the method.

github-actions bot added the core label Apr 30, 2024

nastra mentioned this pull request Apr 30, 2024

Spark: Bypass Spark's ViewCatalog API when replacing a view #9596

Merged

nastra force-pushed the view-assign-fresh-ids-for-schema branch 2 times, most recently from 0a21640 to 6d64913 Compare April 30, 2024 12:51

github-actions bot added the spark label Apr 30, 2024

nastra force-pushed the view-assign-fresh-ids-for-schema branch from 6d64913 to f378c67 Compare April 30, 2024 13:35

nastra commented Apr 30, 2024

View reviewed changes

nastra requested a review from rdblue April 30, 2024 14:58

nastra requested a review from amogh-jahagirdar June 6, 2024 09:52

rdblue reviewed Jun 20, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated Show resolved Hide resolved

rdblue requested changes Jun 20, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java Outdated Show resolved Hide resolved

nastra force-pushed the view-assign-fresh-ids-for-schema branch 2 times, most recently from 5a4e96c to 5c05680 Compare June 21, 2024 15:01

github-actions bot added the stale label Nov 2, 2024

nastra added not-stale and removed stale labels Nov 4, 2024

nastra force-pushed the view-assign-fresh-ids-for-schema branch from 5c05680 to 0214536 Compare December 4, 2024 06:57

amogh-jahagirdar approved these changes Dec 9, 2024

View reviewed changes

nastra requested a review from rdblue December 9, 2024 13:12

nastra force-pushed the view-assign-fresh-ids-for-schema branch from 0214536 to 8d71038 Compare December 9, 2024 13:30

rdblue reviewed Dec 9, 2024

View reviewed changes

nastra force-pushed the view-assign-fresh-ids-for-schema branch from 8d71038 to edffe00 Compare December 9, 2024 20:43

nastra requested a review from rdblue December 10, 2024 07:16

Core: Assign fresh IDs to view schema

00a3aa1

nastra force-pushed the view-assign-fresh-ids-for-schema branch from edffe00 to 00a3aa1 Compare December 11, 2024 09:41

nastra closed this Dec 11, 2024

nastra reopened this Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Assign fresh IDs to view schema #10253

Core: Assign fresh IDs to view schema #10253

nastra commented Apr 30, 2024 •

edited

Loading

nastra Apr 30, 2024

rdblue left a comment

github-actions bot commented Nov 2, 2024

amogh-jahagirdar left a comment

amogh-jahagirdar Dec 9, 2024

nastra Dec 9, 2024

rdblue Dec 9, 2024

rdblue Dec 9, 2024

nastra Dec 10, 2024

rdblue Dec 9, 2024

nastra Dec 10, 2024

rdblue Dec 9, 2024

		Types.NestedField.optional(1, "new_id", Types.IntegerType.get(), "some ID"),
		Types.NestedField.optional(2, "new_data", Types.StringType.get(), "some data"))

Core: Assign fresh IDs to view schema #10253

Are you sure you want to change the base?

Core: Assign fresh IDs to view schema #10253

Conversation

nastra commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

rdblue left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2024

amogh-jahagirdar left a 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

nastra commented Apr 30, 2024 •

edited

Loading