[iceberg] Introduce feature to migrate table from iceberg to paimon #4639

LsomeYeah · 2024-12-04T09:38:04Z

Purpose

Linked issue: close #xxx

Paimon has supported generating Iceberg compatible metadata, so that paimon tables can be consumed directly by Iceberg readers. Now paimon try to support an action or a procedure to support migrating iceberg table to paimon table.

The general implementation idea of this feature includes the following steps:

read snapshot metadata of iceberg latest snapshot
get all data files used by iceberg latest snapshot(existing of deleted kind file will cause exception or be filtered)
migrate these data files to paimon

This pr supports basic ability to migrating iceberg table to paimon, including:

core implementation of migrating, supported iceberg catalog type: hadoop-catalog
UT cases

Procedure or action is not included in this pr.

Tests

org.apache.paimon.iceberg.IcebergMigrateTest

API and Format

Documentation

JingsongLi · 2024-12-04T09:58:30Z

Please write [WIP] in PR title.

tsreaper · 2024-12-10T06:53:48Z

paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergMigrator.java

+        this.paimonCatalog = paimonCatalog;
+        this.paimonFileIO = paimonCatalog.fileIO();
+        this.paimonDatabaseName = paimonDatabaseName;
+        this.paimonTableNameame = paimonTableNameame;


Suggested change

this.paimonTableNameame = paimonTableNameame;

this.paimonTableName = paimonTableName;

tsreaper · 2024-12-10T06:54:50Z

paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergMigrator.java

+        Schema paimonSchema = icebergSchemaToPaimonSchema(icebergMetadata);
+        Identifier paimonIdentifier = Identifier.create(paimonDatabaseName, paimonTableNameame);
+
+        paimonCatalog.createDatabase(paimonDatabaseName, false);


Why false? User must migrate to a non-existing database?

tsreaper · 2024-12-10T07:00:57Z

paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergMigrator.java

+
+    private List<IcebergManifestFileMeta> checkAndFilterManifestFiles(
+            List<IcebergManifestFileMeta> icebergManifestFileMetas) {
+        if (!ignoreDelete) {


Why do we need such an option? If there are deletion vectors in Iceberg, and user uses this option, then the resulting data will be incorrect. Incorrect data are useless to the users.

tsreaper · 2024-12-10T07:53:17Z

paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

@@ -190,6 +199,70 @@ private static Object toTypeObject(DataType dataType, int fieldId, int depth) {
        }
    }

+    public DataType getDataType() {


This class already has a dataType member. Check if dataType is null, if not just return that object, otherwise calculate data type from type string.

# Conflicts: # paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

…manifest entry

…ain metadata path

…tead from iceberg catalog # Conflicts: # paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory

… is not null

tsreaper · 2024-12-19T09:27:40Z

paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

+                !simpleType.contains(delimiter)
+                        ? simpleType
+                        : simpleType.substring(0, simpleType.indexOf(delimiter));
+        switch (typePrefix) {


Now that you have calculated dataType with these code, store the result in dataType so we can directly use it next time.

tsreaper · 2024-12-19T09:51:00Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrator.java

+            if (meta.content() == IcebergManifestFileMeta.Content.DELETES) {
+                throw new RuntimeException(
+                        "IcebergMigrator don't support analyzing manifest file with 'DELETE' content.");
+            }


NIT: Preconditions.checkArgument(meta.content() != IcebergManifestFileMeta.Content.DELETES)

tsreaper · 2024-12-19T09:51:12Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrator.java

+            if (meta.content() != IcebergDataFileMeta.Content.DATA) {
+                throw new RuntimeException(
+                        "IcebergMigrator don't support analyzing iceberg delete file.");
+            }


Same as above.

tsreaper · 2024-12-24T03:04:36Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrator.java

+    public void deleteOriginTable(boolean delete) throws Exception {}
+
+    @Override
+    public void renameTable(boolean ignoreIfNotExists) throws Exception {}


Why don't you implement these?

tsreaper · 2024-12-24T03:11:11Z

paimon-core/src/main/java/org/apache/paimon/migrate/FileMetaUtils.java

+    public static List<DataFileMeta> construct(
+            List<IcebergDataFileMeta> icebergDataFileMetas,
+            FileIO fileIO,
+            Table paimonTable,
+            Path newDir,
+            Map<Path, Path> rollback) {


This method is only related to Iceberg, while other methods in this utility class are quite versatile. Move this method to IcebergMigrator.

tsreaper · 2024-12-24T03:13:03Z

paimon-core/src/test/java/org/apache/paimon/iceberg/migrate/IcebergMigrateTest.java

+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+/** Tests for {@link IcebergMigrator}. */
+public class IcebergMigrateTest {


This is quite a complex feature. I would add a random test.

tsreaper · 2024-12-24T03:14:19Z

...hive-catalog/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrateHiveMetadata.java

+import org.slf4j.LoggerFactory;
+
+/** Get iceberg table latest snapshot metadata in hive. */
+public class IcebergMigrateHiveMetadata implements IcebergMigrateMetadata {


Do you have tests for this class?

test for hive metadata is not included in this pr, I'll move it to next pr.

…ead throwing exception manually

…it in subclass

… contained in other pr

tsreaper · 2024-12-31T09:21:47Z

paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

+        String simpleType = type.toString();
+        String delimiter = "(";
+        if (simpleType.contains("[")) {
+            delimiter = "[";
+        }
+        String typePrefix =
+                !simpleType.contains(delimiter)
+                        ? simpleType
+                        : simpleType.substring(0, simpleType.indexOf(delimiter));
+        switch (typePrefix) {
+            case "boolean":
+                dataType = new BooleanType(!required);
+                break;


NIT: Extract this into a separated method. This can remove all the breaks.

tsreaper · 2024-12-31T09:23:53Z

...on-core/src/main/java/org/apache/paimon/iceberg/manifest/IcebergManifestEntrySerializer.java

+
+    public IcebergManifestEntry fromRow(InternalRow row, IcebergManifestFileMeta meta) {
+        IcebergManifestEntry.Status status = IcebergManifestEntry.Status.fromId(row.getInt(0));
+        long snapshotId = !row.isNullAt(1) ? row.getLong(1) : meta.addedSnapshotId();


Suggested change

long snapshotId = !row.isNullAt(1) ? row.getLong(1) : meta.addedSnapshotId();

long snapshotId = row.isNullAt(1) ? meta.addedSnapshotId() : row.getLong(1);

tsreaper · 2024-12-31T09:47:58Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrateHadoopMetadata.java

+            LOG.warn(
+                    "exception occurred when deleting origin table, exception message:{}",
+                    e.getMessage());


Suggested change

LOG.warn(

"exception occurred when deleting origin table, exception message:{}",

e.getMessage());

LOG.warn(

"exception occurred when deleting origin table", e);

In this way the stack trace is also logged.

tsreaper · 2024-12-31T09:48:51Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrateHadoopMetadata.java

+        } catch (IOException e) {
+            throw new RuntimeException(
+                    "read iceberg version-hint.text failed. Iceberg metadata path: "
+                            + icebergMetaPathFactory.metadataDirectory());
+        }


Why not add e as cause? It will be difficult to debug without stack trace.

tsreaper · 2024-12-31T09:50:18Z

paimon-core/src/main/java/org/apache/paimon/iceberg/migrate/IcebergMigrator.java

+        LOG.info("Last step: rename.");
+        LOG.info("Iceberg migrator do not rename table now.");


No need for this log. Also why Iceberg migrator ignore this method? Add comments.

…ting iceberg datatype.

tsreaper · 2025-01-09T03:17:47Z

paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

+    }
+
+    @JsonIgnore
+    public DataType getDataTypeFromType() {


Suggested change

public DataType getDataTypeFromType() {

private DataType getDataTypeFromType() {

tsreaper · 2025-01-09T03:18:27Z

paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

@@ -190,6 +199,80 @@ private static Object toTypeObject(DataType dataType, int fieldId, int depth) {
        }
    }

+    @JsonIgnore
+    public DataType getDataType() {


Replace the old dataType method with this one.

JingsongLi changed the title ~~[iceberg] Introduce procedure to migrate table from iceberg to paimon~~ [WIP][iceberg] Introduce procedure to migrate table from iceberg to paimon Dec 4, 2024

LsomeYeah force-pushed the migrate-iceberg branch from 7e6b49f to 898eed1 Compare December 5, 2024 05:07

LsomeYeah changed the title ~~[WIP][iceberg] Introduce procedure to migrate table from iceberg to paimon~~ [iceberg] Introduce feature to migrate table from iceberg to paimon Dec 9, 2024

LsomeYeah mentioned this pull request Dec 9, 2024

[iceberg] introduce procedure for migrating iceberg table to paimon #4664

Closed

tsreaper reviewed Dec 10, 2024

View reviewed changes

LsomeYeah added 16 commits December 13, 2024 11:28

first version

a7601c6

# Conflicts: # paimon-core/src/main/java/org/apache/paimon/iceberg/metadata/IcebergDataField.java

[iceberg] inherit seq num from manifest file when seq num is null in …

7aef9fe

…manifest entry

[iceberg] change the type of snapshotId to long

88cd737

support more data types

0679c3c

second version and test

021a407

fix bug for type conversion

4ee8126

add unit tests

1eb789b

[mark] unit tests including add and rename column

f3a9d5b

fix test

4bf44e9

add doc and add dependency in paimon-core/pom.xml

eaf0efe

resolve conflicts

ff8db0a

fix check style

1dd7925

[improve] get iceberg metadata from iceberg table instead from a cert…

625b67b

…ain metadata path

[improve] get iceberg metadata from IcebergMigrateMetadataFactory ins…

98a4ec3

…tead from iceberg catalog # Conflicts: # paimon-core/src/main/resources/META-INF/services/org.apache.paimon.factories.Factory

[improve] allow migrate to existing database

6f2fef8

[improve][core] get datatype directly if datatype in IcebergDataField…

a6fdc51

… is not null

LsomeYeah force-pushed the migrate-iceberg branch from cfa7ae2 to a6fdc51 Compare December 13, 2024 03:30

LsomeYeah added 2 commits December 13, 2024 11:44

[core] change scope for iceberg dependency in paimon-core to test

0c0bb89

[improve][core] remove ignore-delete conf

f73ecb5

tsreaper reviewed Dec 24, 2024

View reviewed changes

LsomeYeah added 4 commits December 26, 2024 15:47

[core] store the iceberg field datatype when computing over

a11769f

[core] remove construct to IcebergMigrator and use Preconditions inst…

92e283a

…ead throwing exception manually

[core] add deleteOriginTable in IcebergMigrateMetadata and implement …

ab74c4a

…it in subclass

[core] implement 'renameTable' again for a independent commit

0fd196f

LsomeYeah added 2 commits December 26, 2024 17:05

[core][test] add a test which using random data

67ee959

[core][hive] remove contents about iceberg hive-catalog which will be…

3911175

… contained in other pr

tsreaper reviewed Dec 31, 2024

View reviewed changes

LsomeYeah added 3 commits January 7, 2025 15:23

[core] throwing exception and logging better. optimize code about get…

ffe2a02

…ting iceberg datatype.

[core] implement rename table

41ded21

[core][fix] avoid json serializer using wrong getters

1f74505

tsreaper reviewed Jan 9, 2025

View reviewed changes

LsomeYeah added 3 commits January 9, 2025 14:25

[core] modify dataType() in IcebergDataField

90cfcae

[core][test] remove content about orc and avro format

3946165

[core] alter access permisssion of getDataTypeFromType()

927a602

tsreaper approved these changes Jan 9, 2025

View reviewed changes

tsreaper merged commit efe2841 into apache:master Jan 9, 2025
12 checks passed

LsomeYeah mentioned this pull request Jan 10, 2025

[iceberg] support migration for iceberg hive-catalog and introduce flink procedure and action #4878

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iceberg] Introduce feature to migrate table from iceberg to paimon #4639

[iceberg] Introduce feature to migrate table from iceberg to paimon #4639

LsomeYeah commented Dec 4, 2024 •

edited

Loading

JingsongLi commented Dec 4, 2024

tsreaper Dec 10, 2024

tsreaper Dec 10, 2024

tsreaper Dec 10, 2024

tsreaper Dec 10, 2024

tsreaper Dec 19, 2024

tsreaper Dec 19, 2024

tsreaper Dec 19, 2024

tsreaper Dec 24, 2024

tsreaper Dec 24, 2024

tsreaper Dec 24, 2024

tsreaper Dec 24, 2024

LsomeYeah Dec 26, 2024

tsreaper Dec 31, 2024

tsreaper Dec 31, 2024

tsreaper Dec 31, 2024

tsreaper Dec 31, 2024

tsreaper Dec 31, 2024

tsreaper Jan 9, 2025

tsreaper Jan 9, 2025

	this.paimonTableNameame = paimonTableNameame;
	this.paimonTableName = paimonTableName;

	long snapshotId = !row.isNullAt(1) ? row.getLong(1) : meta.addedSnapshotId();
	long snapshotId = row.isNullAt(1) ? meta.addedSnapshotId() : row.getLong(1);

		LOG.info("Last step: rename.");
		LOG.info("Iceberg migrator do not rename table now.");

	public DataType getDataTypeFromType() {
	private DataType getDataTypeFromType() {

[iceberg] Introduce feature to migrate table from iceberg to paimon #4639

[iceberg] Introduce feature to migrate table from iceberg to paimon #4639

Conversation

LsomeYeah commented Dec 4, 2024 • edited Loading

Purpose

Tests

API and Format

Documentation

JingsongLi commented Dec 4, 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

LsomeYeah commented Dec 4, 2024 •

edited

Loading