Core: Add TypeUtil.project() for Explicit Projection #2952

RussellSpitzer · 2021-08-07T18:57:34Z

Previously we only had TypeUtil.select which would select all nested
elements of selected structs. This made it difficult to project empty
structs since they would have no selected leaf elements. The new
TypeUtil.project instead takes only the elements which are specifically
requested with no automatic selection of nested elements.

rdblue · 2021-08-09T16:29:31Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

  PruneColumns(Set<Integer> selected) {
    Preconditions.checkNotNull(selected, "Selected field ids cannot be null");
    this.selected = selected;
+    this.onlySelected = false;


What about this(selected, false);?

Yep of course

rdblue · 2021-08-09T16:38:30Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

+  /**
+   * Visits a schema and returns only those element's whose id's have been passed as selected
+   * @param selected ids of elements to return
+   * @param onlySelected whether to return only elements which have been selected, no sub elements


This wording seems a bit unclear to me since the purpose of this class is to return only the selected fields. I think that it misses the context that this affects the fields that are selected when a struct itself is selected.

What about renaming this to selectFullStructs or selectEmptyStructs? Then the javadoc would be "whether explicitly selected structs should contain all fields or no fields".

After thinking about this and reading more of the PR, I think that the boolean should be selectFullTypes so that it is clear how maps and lists are handled. Additionally, when selectFullTypes is false, only struct IDs should be supported. Maps and lists should not be explicitly selected or else there is a problem.

rdblue · 2021-08-09T16:50:54Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

-        } else {
-          selectedFields.add(
-              Types.NestedField.required(field.fieldId(), field.name(), projectedType, field.doc()));
+      boolean includeField = !onlySelected || selected.contains(field.fieldId());


I don't think this is right. I think that the only thing we need to change is the logic for selecting a struct itself, in field. If a field makes it to this point because it is in fieldResults then it (or one of its children) was selected.

For example, if the schema is 1: id bigint, 2: location struct<3: lat float, 4: long float, 5: alt float> and selected includes {3, 4} then the schema returned must include both lat and long fields, even if location is explicitly selected and empty structs should be returned.

The behavior difference is in the case of selected={2, 3, 4}:

When selecting empty structs, the result should be 2: location struct<3: lat float, 4: long float>

When selecting full structs, the result should be 2: location struct<3: lat float, 4: long float, 5: alt float>

Similarly, if selected={2} the results should be:

When selecting empty structs, the result should be 2: location struct<>

When selecting full structs, the result should be 2: location struct<3: lat float, 4: long float, 5: alt float>

So I don't think we need to check onlySelected when considering fields. This should not be a way to prune explicitly selected fields.

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

rdblue · 2021-08-22T17:19:38Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

+    this.selectFullTypes = selectFullTypes;
+  }
+
+  PruneColumns(Set<Integer> selected) {


Since this is package-private, can we remove it and always explicitly pass selectFullTypes?

rdblue · 2021-08-22T17:23:54Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java


-  PruneColumns(Set<Integer> selected) {
+  /**
+   * Visits a schema and returns only those element's whose id's have been passed as selected


I think you may want to rephrase this because the part about "only those elements whose ids have been passed as selected" is a bit unclear to me. I would say "Visits a schema and returns only the fields selected by the ID set." Then in the description you could get more specific about selectFullTypes: "when true, selecting a nested type selects all subfields. otherwise, select produces empty structs." And you can also say that selecting maps and lists directly when selectFullTypes is enabled is ambiguous and therefore not allowed

rdblue · 2021-08-22T17:29:46Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

+        return projectSelectedStruct(fieldResult);
+      } else {
+        Preconditions.checkArgument(selectFullTypes || !field.type().isNestedType(),
+            "Cannot select partial List or Map types explicitly");


Should this include the ID and field that was selected to give more context?

rdblue · 2021-08-22T17:32:11Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

  }
+
+  private ListType projectList(ListType list, Type elementResult) {
+    Preconditions.checkArgument(elementResult != null);


Can you add friendly error messages to all of the preconditions?

rdblue · 2021-08-22T17:36:29Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

+   * Project extracts particular fields from a schema. Unlike
+   * {@link TypeUtil#select(Schema, Set)}, project will pick out only
+   * the fields enumerated, this means no sub fields will be selected
+   * unless they are explicitly requested.


Nit: line wrapping here is odd. Could you wrap at 120 chars?

rdblue · 2021-08-22T17:36:59Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

  }

+  /**
+   * Project extracts particular fields from a schema. Unlike


We typically have a short high-level description (hopefully on a single line) and then a paragraph break followed by other relevant information.

rdblue · 2021-08-22T17:38:18Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

+   * Project extracts particular fields from a schema. Unlike
+   * {@link TypeUtil#select(Schema, Set)}, project will pick out only
+   * the fields enumerated, this means no sub fields will be selected
+   * unless they are explicitly requested.


You might add "... unless they are explicitly requested. Structs that are explicitly projected are empty unless sub-fields are explicitly projected. Maps and lists cannot be explicitly selected in fieldIds."

rdblue · 2021-08-22T17:39:21Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

+        return new Schema(result.fields());
+      }
+    }
+    return new Schema();


If no fields are selected, but the schema had aliases, this will drop the aliases. Can you ensure we always retain the aliases?

rdblue · 2021-08-22T17:40:18Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

+    Preconditions.checkNotNull(schema, "Schema cannot be null");
+
+    Types.StructType result = project(schema.asStruct(), fieldIds);
+    if (Objects.equals(schema.asStruct(), result)) {


Minor: If schema is non-null from the check above, then schema.asStruct() must also be non-null. So you should be able to use equals instead if you want.

rdblue · 2021-08-22T17:41:30Z

api/src/main/java/org/apache/iceberg/types/TypeUtil.java

+    Preconditions.checkNotNull(fieldIds, "Field ids cannot be null");
+
+    Type result = visit(struct, new PruneColumns(fieldIds, false));
+    if (struct == result) {


Should this be .equals? We used to use identity, but there are build warnings so now we just use equals.

Looks like this wasn't updated.

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

rdblue · 2021-08-22T17:45:41Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+  }
+
+  @Test
+  public void testProjectList() {


Looks good.

rdblue · 2021-08-22T17:46:02Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+
+  @Test
+  public void testProjectMap() {
+    // We can't partially project keys


I'd say that we don't because that changes key equality.

rdblue · 2021-08-22T17:47:11Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+                    optional(100, "x", Types.IntegerType.get()),
+                    optional(101, "y", Types.IntegerType.get())),
+                Types.StructType.of()))));
+    Schema actualDepthOne = TypeUtil.project(schema, Sets.newHashSet(10, 13, 14, 100, 101));


100 and 101 should be implicitly projected because any part of the key is projected.

rdblue · 2021-08-22T17:47:46Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+  }
+
+  @Test
+  public void testSelect() {


Looks good.

RussellSpitzer · 2021-08-25T23:25:55Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+                )))));
+
+    Schema actualDepthTwo = TypeUtil.project(schema, Sets.newHashSet(11, 12, 15, 17));
+    Schema actualDepthTwoChildren = TypeUtil.project(schema, Sets.newHashSet(11, 17));


Added another "Child only" projection here

RussellSpitzer · 2021-08-25T23:26:19Z

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

+                    optional(101, "y", Types.IntegerType.get())),
+                Types.StructType.of()))));
+    Schema actualDepthOne = TypeUtil.project(schema, Sets.newHashSet(10, 13, 14, 100, 101));
+    Schema actualDepthOneNoKeys = TypeUtil.project(schema, Sets.newHashSet(10, 13, 14));


Implicit projection of keys here

rdblue · 2021-08-29T19:07:44Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

+   * Visits a schema and returns only the fields selected by the id set.
+   * @param selected ids of elements to return
+   * @param selectFullTypes when true, selecting a nested type selects all subfields. When false selecting list or
+   *                        map types is undefined and forbidden.


Minor: I normally opt to explain behavior in the method description and not in argument descriptions. Argument descriptions should state what the argument controls, but not all the implications. Otherwise, the argument descriptions are very long and reading the description doesn't cover the full behavior of the method. I'd probably change this to just "when true, selecting a nested type selects all subfields". And add a paragraph above that explains the behavior for structs, maps, and lists.

rdblue · 2021-08-29T19:09:58Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

-      // fields selected.
-      return fieldResult;
+      // This field wasn't selected but a subfield was so include that
+      return  fieldResult;


Nit: this line didn't change except for an accidental new space.

rdblue · 2021-08-29T19:17:28Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

-        return list;
-      } else if (list.isElementOptional()) {
-        return Types.ListType.ofOptional(list.elementId(), elementResult);
+      if (list.elementType().isStructType() && !selectFullTypes) {


I think this should be slightly different:

if (selectFullTypes) { return list; } else if (list.elementType().isStructType()) { StructType projectedStruct = projectSelectedStruct(elementResult); return projectList(list, projectedStruct); } else { Precondtions.checkArgument(list.elementType().isPrimitive(), "Cannot directly project list or map"); return list; }

I think that makes it more understandable because selectFullTypes is handled first and so we know that the remaining cases are not selecting full types. It also handles lists of lists because of the argument check. I think that this was allowing an inner list to be selected directly.

You should probably add test cases for list of lists or map of maps.

rdblue · 2021-08-29T19:17:50Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

-        return map;
-      } else if (map.isValueOptional()) {
-        return Types.MapType.ofOptional(map.keyId(), map.valueId(), map.keyType(), valueResult);
+      if (map.valueType().isStructType() && !selectFullTypes) {


Similar to above, I think this should handle selectFullTypes and then go into more cases.

rdblue · 2021-08-29T19:20:34Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

  public Type field(Types.NestedField field, Type fieldResult) {
    if (selected.contains(field.fieldId())) {
-      return field.type();
+      if (field.type().isStructType() && !selectFullTypes) {


Similar to below, it is probably going to be more readable later to handle selectFullTypes first and then move to other cases.

rdblue

Looks nearly there, but I think that a list of lists is going to allow lists to be directly selected.

RussellSpitzer · 2021-09-01T21:40:37Z

@rdblue Added more tests and changed around the list/map/field if statement ordering, should be good to go now

Previously we only had TypeUtil.select which would select all nested elements of selected structs. This made it difficult to project empty structs since they would have no selected leaf elements. The new TypeUtil.project instead takes only the elements which are specifically requested with no automatic selection of nested elements.

rdblue · 2021-09-02T19:30:32Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

      } else {
-        return Types.ListType.ofRequired(list.elementId(), elementResult);
+        Preconditions.checkArgument(list.elementType().isPrimitiveType(),
+            "Cannot explicitly project List or Map types, Field %s of type %s was selected",


Not really a "field" so much as a "list element"

rdblue · 2021-09-02T19:31:12Z

api/src/main/java/org/apache/iceberg/types/PruneColumns.java

      } else {
-        return Types.MapType.ofRequired(map.keyId(), map.valueId(), map.keyType(), valueResult);
+        Preconditions.checkArgument(map.valueType().isPrimitiveType(),
+            "Cannot explicitly project List or Map types, Field %s of type %s was selected",


Nit: I'd replace "field" with "map value"

rdblue

Looks great! I had a couple of nits in error messages, but this is ready to go.

RussellSpitzer · 2021-09-03T02:07:43Z

Tests had the timeout issue again, it has to be unrelated to this patch since this patch doesn't actually change any other used apis so I'm going to merge. The suite also passed before I changed the error messages.

* Core: Add TypeUtil.project() for Explicit Projection Previously we only had TypeUtil.select which would select all nested elements of selected structs. This made it difficult to project empty structs since they would have no selected leaf elements. The new TypeUtil.project instead takes only the elements which are specifically requested with no automatic selection of nested elements.

RussellSpitzer requested a review from rdblue August 7, 2021 18:57

github-actions bot added the API label Aug 7, 2021

rdblue reviewed Aug 9, 2021

View reviewed changes

api/src/main/java/org/apache/iceberg/types/PruneColumns.java Show resolved Hide resolved

rdblue reviewed Aug 22, 2021

View reviewed changes

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java Show resolved Hide resolved

rdblue reviewed Aug 22, 2021

View reviewed changes

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

}

@Test

public void testProjectList() {

Copy link

Contributor

rdblue Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

rdblue reviewed Aug 22, 2021

View reviewed changes

api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java

}

@Test

public void testSelect() {

Copy link

Contributor

rdblue Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

RussellSpitzer commented Aug 25, 2021

View reviewed changes

rdblue reviewed Aug 29, 2021

View reviewed changes

rdblue requested changes Aug 29, 2021

View reviewed changes

RussellSpitzer added 4 commits September 2, 2021 12:17

Handle projection of structs within Maps and Lists

6375737

Reviewer Comments

ffc149a

Fix issue with nested lists and maps

0372d16

RussellSpitzer force-pushed the TypeUtilProject branch from 0d00cc1 to 0372d16 Compare September 2, 2021 17:17

rdblue reviewed Sep 2, 2021

View reviewed changes

rdblue approved these changes Sep 2, 2021

View reviewed changes

Error message change

35a768f

RussellSpitzer merged commit 599ee22 into apache:master Sep 3, 2021

RussellSpitzer deleted the TypeUtilProject branch September 3, 2021 02:08

kbendick mentioned this pull request Nov 2, 2021

Investigate amount of work needed to backport #3240 to 0.12.1 #3443

Closed

RussellSpitzer mentioned this pull request Feb 9, 2022

Losing "element-id" after calling BuildAvroProjection #4072

Closed

Core: Add TypeUtil.project() for Explicit Projection #2952

Core: Add TypeUtil.project() for Explicit Projection #2952

Uh oh!

Conversation

RussellSpitzer commented Aug 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Sep 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Sep 3, 2021

Uh oh!