[#1313][#1471] feat(iceberg): Support struct column for iceberg #1721

TEOTEO520 · 2024-01-25T19:55:33Z

What changes were proposed in this pull request?

add struct column support for iceberg

Why are the changes needed?

Fix: #1313
Fix: #1471

Does this PR introduce any user-facing change?

no

How was this patch tested?

UT added

mchades

The test also need to be added in com.datastrato.gravitino.integration.test.catalog.lakehouse.iceberg.CatalogIcebergIT

mchades · 2024-01-26T02:24:13Z

.../src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ConvertUtil.java

+   * @param field Gravitino field.
+   * @param id
+   * @return


Please add a full comment

OK, I'll add it

mchades · 2024-01-26T02:25:17Z

.../src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ConvertUtil.java

@@ -61,4 +61,22 @@ public static IcebergColumn fromNestedField(Types.NestedField nestedField) {
        .withType(ConvertUtil.formIcebergType(nestedField.type()))
        .build();
  }
+
+  /**
+   * Convert the Gravitino field of Iceberg to the Iceberg column.


It's field of StructType, not Iceberg

OK, I'll fix it

mchades · 2024-01-26T02:28:26Z

.../java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergTypeVisitor.java

+          Arrays.stream(fields)
+              .map(
+                  field -> {
+                    return ConvertUtil.fromGravitinoField(field, 0);


The id is always zero?

It's best to generate this ID according to iceberg's generation rules

The id will not be used in the subsequent logic, visitor.struct() will generate this ID according to iceberg's generation rules.
My idea is that since the generated id here will not be used later, assigning a value of 0 directly to the id will make the code more concise

@Override public Type struct(IcebergTable struct, List<Type> types) { List<IcebergColumn> fields = Arrays.stream(struct.columns()) .map(column -> (IcebergColumn) column) .collect(Collectors.toList()); List<Types.NestedField> newFields = Lists.newArrayListWithExpectedSize(fields.size()); boolean isRoot = root == struct; for (int i = 0; i < fields.size(); i += 1) { IcebergColumn field = fields.get(i); Type type = types.get(i); // for new conversions, use ordinals for ids in the root struct int id = isRoot ? i : getNextId(); if (field.nullable()) { newFields.add(Types.NestedField.optional(id, field.name(), type, field.comment())); } else { newFields.add(Types.NestedField.required(id, field.name(), type, field.comment())); } } return Types.StructType.of(newFields); }

Do you think it is necessary to add generated ID logic in this method, though these ID will never be used?

If it's confirmed that this won't be used, then I think we can pass a 0 to keep the code clean. We should first raise an issue to refactor the previous code and abandon the use of ID.

The 0 of id passed here will never be used in the subsequent logic, so It's not necessary to generate id here.
As you mentioned, it will be much appreciate if you can raise an issue to discuss the refactor of previous code and abandon the use of ID.

jerryshao · 2024-01-26T03:35:48Z

@FANNG1 @Clearvive can you please also help to review.

.../main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/FromIcebergType.java

Clearvive · 2024-01-26T06:59:54Z

.../src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ConvertUtil.java

+  public static IcebergColumn fromGravitinoField(
+      com.datastrato.gravitino.rel.types.Types.StructType.Field field, int id) {
+    return new IcebergColumn.Builder()
+        .withId(id)


The iceberg type ID has generation rules, but it is uncertain whether the underlying layer is useful

The input parameters of this method is StructType.Field, not a table, my understanding of iceberg type ID generation rules is aimed at IcebergTable
I think it's better to put the rules of generating id to the previous layer method (the method that call fromGravitinoField)

Clearvive · 2024-01-26T07:00:46Z

Consider adding relevant tests in CatalogIcebergIT

.../java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergTypeVisitor.java

.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestConvertUtil.java

FANNG1 · 2024-02-05T11:34:52Z

.../src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ConvertUtil.java

@@ -17,10 +18,25 @@ public class ConvertUtil {
   * Convert the Iceberg Table to the corresponding schema information in the Iceberg.
   *
   * @param icebergTable Iceberg table.
-   * @return iceberg schema.
+   * @return Iceberg schema.
   */
  public static Schema toIcebergSchema(IcebergTable icebergTable) {


could you rename icebergTable to gravitinoTable to make it more clear?

.../src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ConvertUtil.java

.../java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergTypeVisitor.java

FANNG1 · 2024-02-05T11:52:06Z

LGTM, except a few minor comments

FANNG1 · 2024-02-05T11:57:21Z

please rebase the latest code. and change the Iceberg document add Struct to Iceberg support column types in docs/lakehouse-iceberg-catalog.md

jerryshao · 2024-02-05T12:35:07Z

So we finally catch the 0.4.0 release, thanks @TEOTEO520 for your work. 😄

…pport-iceberg-struct

yuqi1129 · 2024-02-05T13:31:26Z

@FANNG1
Can you assist in checking if the changes to the documents are acceptable?

TEOTEO520 · 2024-02-05T13:32:31Z

It's my first PR for open source community in my life, thanks to everyone in Gravitino for your advices and selfless help!
@jerryshao @Clearvive @FANNG1 @mchades @qqqttt123 @yuqi1129

FANNG1 · 2024-02-05T13:33:24Z

@FANNG1 Can you assist in checking if the changes to the documents are acceptable?

LGTM, let's wait the CI to finish

…berg (#1721) ### What changes were proposed in this pull request? add struct column support for iceberg ### Why are the changes needed? Fix: #1313 Fix: #1471 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? UT added --------- Co-authored-by: teo <[email protected]>

FANNG1 · 2024-02-05T14:04:52Z

@TEOTEO520 , thanks for your work, wellcome to join Gravitino community, hope you enjoy the opensource journey :)

support struct for iceberg

567aff8

TEOTEO520 changed the title ~~support struct for iceberg~~ [#1313] feat(iceberg): Support struct column for iceberg Jan 25, 2024

qqqttt123 requested a review from mchades January 26, 2024 01:50

mchades requested review from FANNG1 and Clearvive January 26, 2024 02:22

mchades assigned TEOTEO520 Jan 26, 2024

mchades requested a review from qqqttt123 January 26, 2024 02:22

mchades reviewed Jan 26, 2024

View reviewed changes

qqqttt123 changed the title ~~[#1313] feat(iceberg): Support struct column for iceberg~~ [#1313][#1471] feat(iceberg): Support struct column for iceberg Jan 26, 2024

FANNG1 reviewed Jan 26, 2024

View reviewed changes

.../main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/FromIcebergType.java Outdated Show resolved Hide resolved

Clearvive reviewed Jan 26, 2024

View reviewed changes

FANNG1 reviewed Jan 26, 2024

View reviewed changes

.../java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergTypeVisitor.java Outdated Show resolved Hide resolved

FANNG1 reviewed Jan 26, 2024

View reviewed changes

.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestConvertUtil.java Show resolved Hide resolved

teo added 2 commits January 30, 2024 18:17

compelete CatalogIcebergIT and improve code

799255e

use structType for ConvertUtil instead of icebergtable

25e68f3

FANNG1 reviewed Feb 5, 2024

View reviewed changes

modify the comment and drop unneccessary fun

0eff76e

jerryshao added need backport Issues that need to backport to another branch branch-0.4 labels Feb 5, 2024

teo added 2 commits February 5, 2024 20:51

Merge branch 'main' of github.com:TEOTEO520/gravitino into feature/su…

e011a9e

…pport-iceberg-struct

update doc and modify comment

786d877

FANNG1 approved these changes Feb 5, 2024

View reviewed changes

FANNG1 merged commit f63f6f3 into apache:main Feb 5, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1313][#1471] feat(iceberg): Support struct column for iceberg #1721

[#1313][#1471] feat(iceberg): Support struct column for iceberg #1721

TEOTEO520 commented Jan 25, 2024 •

edited by jerryshao

Loading

mchades left a comment

mchades Jan 26, 2024

TEOTEO520 Jan 29, 2024

mchades Jan 26, 2024

TEOTEO520 Jan 29, 2024

mchades Jan 26, 2024

Clearvive Jan 26, 2024

TEOTEO520 Jan 29, 2024 •

edited

Loading

TEOTEO520 Jan 29, 2024

Clearvive Jan 30, 2024

TEOTEO520 Jan 30, 2024

jerryshao commented Jan 26, 2024

Clearvive Jan 26, 2024

TEOTEO520 Jan 29, 2024

Clearvive commented Jan 26, 2024

FANNG1 Feb 5, 2024

FANNG1 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

jerryshao commented Feb 5, 2024

yuqi1129 commented Feb 5, 2024

TEOTEO520 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

[#1313][#1471] feat(iceberg): Support struct column for iceberg #1721

[#1313][#1471] feat(iceberg): Support struct column for iceberg #1721

Conversation

TEOTEO520 commented Jan 25, 2024 • edited by jerryshao Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

mchades 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

TEOTEO520 Jan 29, 2024 • edited Loading

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

jerryshao commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clearvive commented Jan 26, 2024

Choose a reason for hiding this comment

FANNG1 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

jerryshao commented Feb 5, 2024

yuqi1129 commented Feb 5, 2024

TEOTEO520 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

FANNG1 commented Feb 5, 2024

TEOTEO520 commented Jan 25, 2024 •

edited by jerryshao

Loading

TEOTEO520 Jan 29, 2024 •

edited

Loading