feat: migrate from icelake to iceberg-rust #19887

ZENOTME · 2024-12-23T03:06:58Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR migrate icelake to iceberg-rust.
After risingwavelabs/iceberg-rust#10, iceberg-rust can support write, so we can migrate all things from icelake to iceberg-rust.

Checklist

I have written necessary rustdoc comments.
I have added necessary unit tests and integration tests.
I have added test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

ZENOTME · 2024-12-23T03:08:06Z

We should merge risingwavelabs/iceberg-rust#10 first, and after that, I will rebase this PR and convert it into the normal state.

Li0k · 2024-12-23T06:07:53Z

We should merge #10 first, and after that, I will rebase this PR and convert it into the normal state.

Is wrong PR link in the description?

ZENOTME · 2024-12-23T06:10:32Z

We should merge #10 first, and after that, I will rebase this PR and convert it into the normal state.

Is wrong PR link in the description?

Thanks! Good catch!🤣

chenzl25 · 2024-12-24T04:02:58Z

src/connector/src/connector_common/iceberg/jni_catalog.rs

-    async fn update_table(self: Arc<Self>, update_table: &UpdateTable) -> icelake::Result<Table> {
-        execute_with_jni_env(self.jvm, |env| {
-            let request_str = serde_json::to_string(&CommitTableRequest::try_from(update_table)?)?;
-
-            let request_jni_str = env.new_string(&request_str).with_context(|| {
-                format!("Failed to create jni string from request json: {request_str}.")
-            })?;
-
-            let result_json =
-                call_method!(env, self.java_catalog.as_obj(), {String updateTable(String)},
-                &request_jni_str)
-                .with_context(|| {
-                    format!(
-                        "Failed to update iceberg table: {}",
-                        update_table.table_name()
-                    )
-                })?;
-
-            let rust_json_str = jobj_to_str(env, result_json)?;
-
-            let response: CommitTableResponse = serde_json::from_str(&rust_json_str)?;
-


This implementation should be migrated to iceberg-rust JNI as well.

chenzl25

Rest LGTM. Thank you so much for this PR!!!

chenzl25 · 2024-12-24T09:58:10Z

src/connector/src/connector_common/iceberg/jni_catalog.rs

@@ -402,8 +312,57 @@ impl CatalogV2 for JniCatalog {
    }

    /// Update a table to the catalog.
-    async fn update_table(&self, _commit: TableCommit) -> iceberg::Result<TableV2> {
-        todo!()
+    async fn update_table(&self, mut commit: TableCommit) -> iceberg::Result<TableV2> {


TableV2 now can be rename to Table

chenzl25 · 2024-12-24T10:01:02Z

src/connector/src/source/iceberg/mod.rs

@@ -82,7 +82,7 @@ impl IcebergProperties {
            java_catalog_props.insert("jdbc.password".to_owned(), jdbc_password);
        }
        // TODO: support java_catalog_props for iceberg source
-        self.common.load_table_v2(&java_catalog_props).await
+        self.common.load_table(&java_catalog_props).await


Methods contain v2 can be renamed as well

chenzl25 · 2024-12-24T10:06:27Z

src/connector/src/sink/iceberg/mod.rs

+}
+
+#[allow(clippy::type_complexity)]
+enum IcebergWriterDispatch {


I remember we had some discussion on the writer trait. Why do we now use enum instead?

chenzl25 · 2024-12-25T07:50:10Z

src/connector/src/connector_common/iceberg/jni_catalog.rs

+            let table_metadata = response.metadata;
+
+            let file_io = FileIO::from_path(&response.metadata_location)?
+                .with_props(self.config.table_io_configs.iter())


It seems we don't need self.config (BaseCatalogConfig is a struct from icelake) anymore, we can simplify it to a map.

xxchan

Thanks for the work!!

BTW I think splitting this PR into smaller ones (separate refactor and feat) can make it easier to review more carefully:

migrate sink to iceberg-rust, without touching legacy code
deleting icelake code
rename *_v2 to *

But I'm also ok to merge it as long as ci passes since iceberg connector is under heavy development status. 🤪

chenzl25 · 2024-12-26T05:47:06Z

Let's move fast and get rid of icelake as soon as possible, because right now we have some known issues can't be resolved in icelake (like statistics). By migrating to iceberg-rust, we can support more complex nested data type for iceberg engine table, google cloud store integration, and so on.

ZENOTME · 2024-12-26T06:20:34Z

Thanks @chenzl25 ! I will send PR to get rid of all icelake later.

ZENOTME force-pushed the zj/migrate_iceberg_rust branch from deb061e to 84d2463 Compare December 23, 2024 06:36

ZENOTME changed the title ~~feat: migrate from icelake to iceberg-rust~~ (WIP)feat: migrate from icelake to iceberg-rust Dec 23, 2024

ZENOTME marked this pull request as ready for review December 23, 2024 06:37

ZENOTME requested a review from a team as a code owner December 23, 2024 06:37

ZENOTME requested a review from fuyufjh December 23, 2024 06:37

github-actions bot added type/feature Invalid PR Title labels Dec 23, 2024

ZENOTME added ci/run-e2e-iceberg-sink-tests ci/run-e2e-iceberg-engine-tests labels Dec 23, 2024

ZENOTME force-pushed the zj/migrate_iceberg_rust branch 2 times, most recently from b5dac34 to 2a93eba Compare December 23, 2024 16:27

chenzl25 reviewed Dec 24, 2024

View reviewed changes

graphite-app bot requested a review from a team December 24, 2024 06:16

chenzl25 approved these changes Dec 24, 2024

View reviewed changes

use iceberg-rust instead of icelake

6ff4d64

ZENOTME force-pushed the zj/migrate_iceberg_rust branch from db488e8 to 6ff4d64 Compare December 24, 2024 10:31

remove v2

644e6e8

graphite-app bot requested a review from a team December 24, 2024 11:41

chenzl25 mentioned this pull request Dec 25, 2024

feat: support write risingwavelabs/iceberg-rust#10

Merged

ZENOTME changed the title ~~(WIP)feat: migrate from icelake to iceberg-rust~~ feat: migrate from icelake to iceberg-rust Dec 25, 2024

github-actions bot removed the Invalid PR Title label Dec 25, 2024

switch iceberg-rust branch

c942311

ZENOTME force-pushed the zj/migrate_iceberg_rust branch from 32d5892 to c942311 Compare December 25, 2024 03:39

ZENOTME requested review from xxchan and Li0k December 25, 2024 04:35

chenzl25 reviewed Dec 25, 2024

View reviewed changes

xxchan approved these changes Dec 26, 2024

View reviewed changes

chenzl25 added this pull request to the merge queue Dec 26, 2024

Merged via the queue into main with commit fa5e442 Dec 26, 2024
32 of 33 checks passed

chenzl25 deleted the zj/migrate_iceberg_rust branch December 26, 2024 06:03

chenzl25 mentioned this pull request Dec 26, 2024

test: add a complex nested type test #19940

Merged

8 tasks

chenzl25 mentioned this pull request Jan 8, 2025

Tracking: replace icelake with iceberg-rust #17548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate from icelake to iceberg-rust #19887

feat: migrate from icelake to iceberg-rust #19887

ZENOTME commented Dec 23, 2024 •

edited

Loading

ZENOTME commented Dec 23, 2024 •

edited

Loading

Li0k commented Dec 23, 2024

ZENOTME commented Dec 23, 2024

chenzl25 Dec 24, 2024

chenzl25 left a comment

chenzl25 Dec 24, 2024

chenzl25 Dec 24, 2024

chenzl25 Dec 24, 2024

chenzl25 Dec 25, 2024

xxchan left a comment •

edited

Loading

chenzl25 commented Dec 26, 2024

ZENOTME commented Dec 26, 2024

feat: migrate from icelake to iceberg-rust #19887

feat: migrate from icelake to iceberg-rust #19887

Conversation

ZENOTME commented Dec 23, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

ZENOTME commented Dec 23, 2024 • edited Loading

Li0k commented Dec 23, 2024

ZENOTME commented Dec 23, 2024

chenzl25 Dec 24, 2024

Choose a reason for hiding this comment

chenzl25 left a comment

Choose a reason for hiding this comment

chenzl25 Dec 24, 2024

Choose a reason for hiding this comment

chenzl25 Dec 24, 2024

Choose a reason for hiding this comment

chenzl25 Dec 24, 2024

Choose a reason for hiding this comment

chenzl25 Dec 25, 2024

Choose a reason for hiding this comment

xxchan left a comment • edited Loading

Choose a reason for hiding this comment

chenzl25 commented Dec 26, 2024

ZENOTME commented Dec 26, 2024

ZENOTME commented Dec 23, 2024 •

edited

Loading

ZENOTME commented Dec 23, 2024 •

edited

Loading

xxchan left a comment •

edited

Loading