feat: add support of iceberg key encoder by zuston · Pull Request #308 · apache/fluss-rust

zuston · 2026-02-13T03:13:57Z

Purpose

Linked issue: close #xxx

Brief change log

Tests

API and Format

Documentation

Copilot

Pull request overview

This PR adds support for Iceberg key encoding by implementing the IcebergKeyEncoder struct that follows Fluss Java's encoding specifications. The implementation enables Fluss to encode row keys for the Iceberg data lake format.

Changes:

Added new IcebergKeyEncoder implementation with support for scalar types (Int, BigInt, Float, Double, Date, Time, Timestamp, Decimal, String, Char, Binary, Bytes)
Integrated IcebergKeyEncoder into the KeyEncoderFactory to handle Iceberg format
Added comprehensive test coverage for supported data types

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
crates/fluss/src/row/encode/mod.rs	Replaces the "not yet implemented" error for Iceberg format with the new IcebergKeyEncoder implementation
crates/fluss/src/row/encode/iceberg_key_encoder.rs	Complete implementation of the Iceberg key encoder with type validation, encoding logic, and tests for supported types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T08:45:59Z

+            (DataType::Timestamp(_), Datum::TimestampNtz(ts)) => {
+                let micros =
+                    ts.get_millisecond() * 1000 + (ts.get_nano_of_millisecond() as i64 / 1000);
+                micros.to_le_bytes().to_vec()


Potential integer overflow in timestamp conversion. At line 138, ts.get_millisecond() * 1000 could overflow for very large timestamp values. The maximum i64 value is 9,223,372,036,854,775,807, so multiplying a timestamp in milliseconds by 1000 could overflow for timestamps beyond year 292,277,026. Consider using checked_mul() to detect overflow, or documenting this limitation if it's acceptable based on the expected range of timestamps in Iceberg format.

Copilot · 2026-02-13T08:45:59Z

+    fn validate_supported_type(field_type: &DataType) -> Result<()> {
+        match field_type {
+            DataType::Int(_)
+            | DataType::BigInt(_)
+            | DataType::Float(_)
+            | DataType::Double(_)
+            | DataType::Date(_)
+            | DataType::Time(_)
+            | DataType::Timestamp(_)
+            | DataType::Decimal(_)
+            | DataType::String(_)
+            | DataType::Char(_)
+            | DataType::Binary(_)
+            | DataType::Bytes(_) => Ok(()),
+
+            DataType::Array(_) => Err(IllegalArgument {
+                message:
+                    "Array types cannot be used as bucket keys. Bucket keys must be scalar types."
+                        .to_string(),
+            }),
+            DataType::Map(_) => Err(IllegalArgument {
+                message:
+                    "Map types cannot be used as bucket keys. Bucket keys must be scalar types."
+                        .to_string(),
+            }),
+            other => Err(IllegalArgument {
+                message: format!("Unsupported type for Iceberg key encoder: {other}"),
+            }),
+        }
+    }


The type validation is incomplete. Several data types are supported by the FieldGetter but are missing from this validation:

Boolean (DataType::Boolean) - which would be returned as Datum::Bool

TinyInt (DataType::TinyInt) - which would be returned as Datum::Int8

SmallInt (DataType::SmallInt) - which would be returned as Datum::Int16

TimestampLtz (DataType::TimestampLTz) - which would be returned as Datum::TimestampLtz

Without explicit handling for these types, if a user attempts to use them as key fields, they would get a generic "Unsupported type" error at validation time. However, if this is intentional and matches the Java implementation behavior, then these types should be listed explicitly in validation with appropriate error messages explaining they are not supported for Iceberg keys.

Copilot · 2026-02-13T08:45:59Z

+        let bytes: Vec<u8> = match (&self.field_type, value) {
+            (DataType::Int(_), Datum::Int32(v)) => (v as i64).to_le_bytes().to_vec(),
+            (DataType::Date(_), Datum::Date(v)) => (v.get_inner() as i64).to_le_bytes().to_vec(),
+
+            (DataType::Time(_), Datum::Time(v)) => {
+                let micros = v.get_inner() as i64 * 1000;
+                micros.to_le_bytes().to_vec()
+            }
+
+            (DataType::BigInt(_), Datum::Int64(v)) => v.to_le_bytes().to_vec(),
+            (DataType::Float(_), Datum::Float32(v)) => v.0.to_le_bytes().to_vec(),
+            (DataType::Double(_), Datum::Float64(v)) => v.0.to_le_bytes().to_vec(),
+
+            (DataType::Timestamp(_), Datum::TimestampNtz(ts)) => {
+                let micros =
+                    ts.get_millisecond() * 1000 + (ts.get_nano_of_millisecond() as i64 / 1000);
+                micros.to_le_bytes().to_vec()
+            }
+
+            (DataType::Decimal(_), Datum::Decimal(d)) => d.to_unscaled_bytes(),
+            (DataType::String(_), Datum::String(s)) => s.as_bytes().to_vec(),
+            (DataType::Char(_), Datum::String(s)) => s.as_bytes().to_vec(),
+            (DataType::Binary(_), Datum::Blob(b)) => b.as_ref().to_vec(),
+            (DataType::Bytes(_), Datum::Blob(b)) => b.as_ref().to_vec(),
+
+            // FieldGetter uses Datum::String for CHAR, Datum::Blob for BINARY/BYTES.
+            (expected_type, actual) => {
+                return Err(IllegalArgument {
+                    message: format!(
+                        "IcebergKeyEncoder type mismatch: expected {expected_type}, got {actual:?}"
+                    ),
+                });
+            }
+        };


The encode_key function is missing match arms for several data types that could potentially be returned by FieldGetter:

Boolean (DataType::Boolean) - would return Datum::Bool

TinyInt (DataType::TinyInt) - would return Datum::Int8

SmallInt (DataType::SmallInt) - would return Datum::Int16

TimestampLtz (DataType::TimestampLTz) - would return Datum::TimestampLtz

If these types pass validation (e.g., if validation is updated), the match statement would fail at runtime with an "IcebergKeyEncoder type mismatch" error at line 150-154. Consider adding explicit handling for these types in the match statement, even if only to encode them (for numeric types) or to return a more specific error message explaining they are unsupported for Iceberg keys.

charlesdong1991 · 2026-02-28T20:41:23Z

Will you have some time to update the PR following the review? Thanks a lot! @zuston

zuston · 2026-03-02T02:09:38Z

Will you have some time to update the PR following the review? Thanks a lot! @zuston

Sure, will be updated in this week

leekeiabstraction · 2026-05-09T10:48:51Z

@qzyu999 Thank you for offering to finish up the PR. I've pushed but reverted it from this PR as there were build failures from rebase conflict.

leekeiabstraction · 2026-05-09T10:49:47Z

@zuston Note there were CI failure on your last push: https://github.com/apache/fluss-rust/actions/runs/24123013769/job/70381044371?pr=308

Can you please take a look? Also it seems further rebase is needed now.

leekeiabstraction

Added some comments. PTAL

leekeiabstraction · 2026-05-09T10:51:04Z

-    ))
+/// Helper function to create a ListBuilder for a given element Arrow type.
+/// This uses boxed builders to allow dynamic dispatch for nested types.
+fn create_list_builder(


The functions create_list_builder, create_builder_for_type and create_boxed_builder are fairly similar. Can we apply DRY e.g. through generics or macro?

leekeiabstraction · 2026-05-09T10:53:45Z

-    Int32Builder, Int64Builder, StringBuilder, Time32MillisecondBuilder, Time32SecondBuilder,
-    Time64MicrosecondBuilder, Time64NanosecondBuilder, TimestampMicrosecondBuilder,
-    TimestampMillisecondBuilder, TimestampNanosecondBuilder, TimestampSecondBuilder,
+    Int32Builder, Int64Builder, ListBuilder, StringBuilder, StructBuilder,


I am not sure why we have changes for column writer within this PR?

zuston · 2026-05-09T14:36:53Z

@zuston Note there were CI failure on your last push: https://github.com/apache/fluss-rust/actions/runs/24123013769/job/70381044371?pr=308

Can you please take a look? Also it seems further rebase is needed now.

sure, let me take a look tomorrow.

luoyuxia requested a review from Copilot February 13, 2026 08:40

Copilot started reviewing on behalf of luoyuxia February 13, 2026 08:40 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

charlesdong1991 mentioned this pull request Feb 24, 2026

feat(iceberg): add IcebergBinaryRowWriter #366

Merged

feat: add support of iceberg key encoder

b0c6359

zuston force-pushed the icebergkey branch from dd3c71e to b0c6359 Compare April 8, 2026 05:45

Junfan Zhang added 2 commits April 8, 2026 14:15

update

dcf34ee

add support of array<row>

04b3a08

leekeiabstraction force-pushed the icebergkey branch 2 times, most recently from e3addf9 to 04b3a08 Compare May 9, 2026 10:45

leekeiabstraction reviewed May 9, 2026

View reviewed changes

Conversation

zuston commented Feb 13, 2026

Purpose

Brief change log

Tests

API and Format

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

charlesdong1991 commented Feb 28, 2026

Uh oh!

zuston commented Mar 2, 2026

Uh oh!

leekeiabstraction commented May 9, 2026

Uh oh!

leekeiabstraction commented May 9, 2026

Uh oh!

leekeiabstraction left a comment

Choose a reason for hiding this comment

Uh oh!

leekeiabstraction May 9, 2026

Choose a reason for hiding this comment

Uh oh!

leekeiabstraction May 9, 2026

Choose a reason for hiding this comment

Uh oh!

zuston commented May 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants