iceberg orc use orc-core-nohive ,flink use orc-core. They will have some conflicts, so I can not use sql to write data into hive tables

we need to close the catalog in the end

this method is getting very long. maybe extract this fileFormat logic to a util class/method?

to be consistent, I would suggest we also pass in the icebergDbName as arg.

nit: it seems that Iceberg convention is to have more full name. like Db -> Database. sam thing for the hiveSourceDbName above

one probably slightly simpler approach is to use List to avoid the if-else. First add the levels from baseNamespace. Then add the icebergDbName. Then toArray

Actually, I found a simpler one-liner.

Namespace.of(ArrayUtils.concat(baseNamespace.levels(), new String[] {DATABASE}))

I am concerned about how this works for huge tables with millions of files.

• collecting all of them into a single list can cause memory pressure.
• writing all of them into a single manifest file may also hurt read performance for partitioned tables, as we can't filter manifest files by partition value ranges in manifest list file.

Maybe we should commit the files by batches. Each Hive partition is a batch and all data files under one Hive partitions get written into a single manifest file. We can use AppendFiles#appendManifest API too.

thanks for your suggesion，I will refactor this logic

• collecting all of them into a single list can cause memory pressure.

I looked at the code again, the listStatus method of hadoop filesystem use ArrayList to construct the FileStatus array.

  public FileStatus[] listStatus(Path f, PathFilter filter) 
                                   throws FileNotFoundException, IOException {
    ArrayList<FileStatus> results = new ArrayList<FileStatus>();
    listStatus(results, f, filter);
    return results.toArray(new FileStatus[results.size()]);
  }

although there is the org.apache.hadoop.fs.FileSystem#listStatusIterator method, the listStatus method is still called in the internal of FileSystem#listStatusIterator method, so when we get the file list of hive, even if we use Iterator, I think it is still useless.

After the refactoring of emitting ManifestFile per partition, we are only listing the files under one partition. Then I won't be concerned for the list. Previously, we are collecting all files under a table.

extract this to a separate method migrateUnpartitionedTable?

As mentioned in the another comment, we can parallelize the processing of each Hive partition and generate a manifest file for each partition on taskmanagers. That is probably the more expensive part of migration. Then on the jobmanager, we can just keep appendManifest and commit once.

nit: Map -> Mapper? Map gives the impression of Java Collection.

Here is the Flink DataStream map API

map(MapFunction<T, R> mapper)

could a table have mixed file formats?

I lookup the hive doc, when we create a hive table , specify the table format by STORED AS file_format, and the table format is table level properties. I think we cannot create different formats for the same table.

One can change the InputFormat/OutputFormat, but that will result in a "dead" table. So it is ok to expect only a single file format.

nit: there is a withFormat overload that takes the enum directly. we don't have to call .name()

nit: for better readability, should we rename map to partitionSpec?

nit: drop the TestMigrateAction.

we need to close HiveMetaStoreClient, right?

should we create HiveCatalog once in beforeClass?

why override it? what is the reason to enableCheckpointing and set parallelism to 1?

because we use StreamExecutionEnvironment in the MigrateAction, so I use StreamExecutionEnvironment to construct the StreamTableEnvironment .

the parallelism and enableCheckpointing is my mistake ,I remove it

Can you elaborate a little why do we need StreamExecutionEnvironment?

in the future, we will use StreamExecutionEnvironment and DataStream to do streaming and batch jobs. the DataSet will be deprecated, (here)

This means that, in the long run, the DataSet API will be deprecated and subsumed by the DataStream API and the Table API/SQL (FLIP-131).

I am aware of the DataSet deprecation in the future. Let me ask in a diff way. Why FlinkTestBase#getTableEnv doesn't work for this case? TableEnvironment interface is a higher abstraction than DataSet/DataStream.

because the FlinkTestBase#getTableEnv only provide a TableEnvironment, but in the MigrateAction,we need a StreamExecutionEnvironment to get DataStream by StreamExecutionEnvironment#fromElements.

Simply , we need to do the migrate by flink api instead of flink sql

can we test the case with multiple partitions?

if we are going to create 1 manifest per partition, then we can also add an assertion on allManifests

nit: "Failed to migrate"?

nit: add some logging onFailure

nit: move this into the try block?

in the Spark path we default to adding a mapping if none is specified. I think this is probably a good idea here as well.

I'm not sure if this is an issue in Hive, but in Spark there can be differences between file layout and schema of the table which lets files possibly not match the Iceberg schema. Adding in the name mapping protects against these cases

yes, I add the name mapping.

One thing that we worried about here, and I'm not sure you have the same issue here, is what happens if a Hive writer is attempting to write while these migrations are occurring. We don't want to end up with dead files that a writer thinks are committed. To avoid this in the Spark Action, we move the catalog entry to a backup identifier so that all modifications will fail until the migration is complete. Basically making the old entry unavailable until we are sure it is either a Iceberg Table or we have aborted and we move it back as it's original config.

I read the code of Spark3MigrateAction, but I am not familiar with spark, I don’t know if I understand it correctly. In Spark3MigrateAction, source catalog and destination catalog, source table name and destination table name are the same, so after we had migrated a non-iceberg table to the iceberg table, the read and write operations using spark may not require any changes.

But for hive table and iceberg table, some changes will be needed. For example, we are using flink to read and write hive table, the sql like this: SELECT * FROM hivecatalog.hivedb.hivetable; After the migration is completed, the sql needs to be modified to select * from iceberg_catalog.iceberg_db.iceberg_table, so when users migrate the hive table to iceberg, they need to stop the writing hive job first, then migrate, and finally modify the logic to reading and writing iceberg table. So I think even if we renamed the source hive table, it does not make much sence to the user, because the user still needs to manually stop and modify the job of writing hive to write iceberg.

The issue is that, say the user is writing to hivetable and while a write is in progress you start the migrate operation. The write finishes and commits, but the migrate command will not pick up these new files since they were created after the migrate scanned the partitions for existing datafiles.

So you can have a successful write to the table which doesn't get migrated, I'm not sure if this is ok or not.

So my idea is that in order to prevent this case, we recommend that users should stop writing hive job first before migrating, then do the migration, and finally modify the writing and reading job of hive.

if migrate failed, we will do the clean work on catch block, If unfortunately the clean failed, the user may need to manually clean the iceberg table and manifests.

I will add some comments on MigrateAction,what do you think ?

Maybe move this into a utility class now that it's public static?

yes, I move it to SimpleDataUtil,this method can be used for the assertion of unsorted list，I have encountered similar needs in other PR

We have something like it in the Hive tests as well, and I would guess Spark has one too 😄
Do we have some other place even more accessible?

@pvary , Could you give me some cases in the Hive module? Let me see if we can move them to data or core module

@zhangjun0x01: We have 2 flavors for checking the contents of the table:

• HiveIcebergTestUtils.validateData(Table table, List expected, int sortBy)
• HiveIcebergTestUtils.validateData(List expected, List actual, int sortBy)

Thanks,
Peter

@pvary I search the spark code , It does have a similar required， but I think we should extract the function in a new PR later ,what do you think?

I am fine with moving this refactoring to another PR.
Thanks, Peter

Here we pass the namespace, database, table , then how about just use a TableIdentifier ?

I think the database,table may be more clear for new iceberg user ,because the TableIdentifier is iceberg internal class,the user need to know how to construct the TableIdentifier,what do you think?

Those database & table could just be simplified by a TableIdentifier.

The ArrayUtils is a Internal API for flink, I will suggest not to use it because it may be changed in the next release.

yes ,I use the ObjectArrays in google guava to replace the ArrayUtils

@zhangjun0x01 Is possible to use the FlinkCatalog to create the table ? Then we don't have to use different code paths to create the iceberg table, I'm concerning that there will be more Catalog that will be introduced in future.