feat: Add equality delete writer #703

ZENOTME · 2024-11-19T13:18:37Z

This PR fix tests and refines the interface based on #372. Thanks, @Dysprosium0626 again for this great job! The fix is included in the extra commit. Feel free for any suggestions. @Xuanwo @liurenjie1024 @sdd @Fokko

liurenjie1024

Thanks @ZENOTME for this pr, generally LGTM! There are some structs I don't understand quite well, please add comment.

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

liurenjie1024 · 2024-11-20T13:53:26Z

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

+
+/// Help to project specific field from `RecordBatch`` according to the column id.
+#[derive(Clone)]
+pub struct ArrowFieldProjector {


I'm quite confusing what's the meaning of each field, please add more comment here.

Also, do we actually need to expose this struct?

Also, do we actually need to expose this struct?

Yes, this struct can help the user to get the projected schema from the original schema. Our lowel writer API requires user to provide the final schema in file writer, see:

iceberg-rust/crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

Line 477 in ac29484

Arc::new(delete_schema),

This is like an enhanced version of this method, which handles nested schema. Also this is too arrow specific, so I would following changes:

Move this struct to arrow module

We could maintain the logic here for now, but it's better to contribute back to arrow community.

I still prefer this to be crate private, as user only need to pass equality_field_id list to build the writer config.

I still prefer this to be crate private, as user only need to pass equality_field_id list to build the writer config.

But how does the user pass the projected schema to the parquet writer?

liurenjie1024 · 2024-11-25T02:23:51Z

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

+        } else {
+            Err(Error::new(
+                ErrorKind::Unexpected,
+                "Equality delete inner writer does not exist",


Suggested change

"Equality delete inner writer does not exist",

"Equality delete inner writer has been closed.",

liurenjie1024 · 2024-11-25T02:24:08Z

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

+        } else {
+            Err(Error::new(
+                ErrorKind::Unexpected,
+                "Equality delete inner writer does not exist",


Suggested change

"Equality delete inner writer does not exist",

"Equality delete inner writer has been closed.",

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

liurenjie1024 · 2024-11-25T02:30:41Z

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

+
+/// Help to project specific field from `RecordBatch`` according to the column id.
+#[derive(Clone)]
+pub struct ArrowFieldProjector {


This is like an enhanced version of this method, which handles nested schema. Also this is too arrow specific, so I would following changes:

Move this struct to arrow module

We could maintain the logic here for now, but it's better to contribute back to arrow community.

I still prefer this to be crate private, as user only need to pass equality_field_id list to build the writer config.

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

liurenjie1024 · 2024-11-25T02:32:17Z

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

+    /// Create a new `DataFileWriterConfig` with equality ids.
+    pub fn new(
+        equality_ids: Vec<usize>,
+        projector: ArrowFieldProjector,


I think user doesn't need to pass this, they just need to pass original iceberg schema.

How about letting the user get projected schema from config🤔 Like:

let config = EqualityDeleteWriterConfig::new(original_schema); let projected_schema = config.projected_schema();

liurenjie1024 · 2024-11-26T08:29:24Z

crates/iceberg/src/arrow/record_batch_projector.rs

+
+/// Help to project specific field from `RecordBatch`` according to the fields id of meta of field.
+#[derive(Clone)]
+pub struct RecordBatchProjector {


We should make this pub(crate), otherwise it would be easy to leak this when export module by mistake.

crates/iceberg/src/arrow/record_batch_projector.rs

liurenjie1024 · 2024-11-26T08:52:35Z

crates/iceberg/src/arrow/record_batch_projector.rs

+                DataType::Float16 | DataType::Float32 | DataType::Float64 => {
+                    return Err(Error::new(
+                        ErrorKind::DataInvalid,
+                        "Delete column data type cannot be float or double",
+                    ));
+                }


Why this check happens here? It means that any field is float then we throw an error, even it's not deletion field? Also as a column batch projector, we should not do this since this is iceberg specific logic.

Yes, my bad. I'm trying to separate this out.

crates/iceberg/src/arrow/record_batch_projector.rs

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

liurenjie1024

Thanks @ZENOTME for this pr! Generally LGTM, just left some minor points

liurenjie1024 · 2024-11-27T06:42:24Z

crates/iceberg/src/arrow/record_batch_projector.rs

+                &field_id_fetch_func,
+                &searchable_field_func,
+            )?
+            .ok_or_else(|| Error::new(ErrorKind::Unexpected, "Field not found"))?;


Suggested change

.ok_or_else(|| Error::new(ErrorKind::Unexpected, "Field not found"))?;

.ok_or_else(|| Error::new(ErrorKind::Unexpected, "Field not found").with_context("field_id", id)?;

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

Sorry, it's approved by mistake

liurenjie1024

Thanks @ZENOTME for this great pr!

* copy from apache#372 * fix test and refine ArrowFieldProjector * add comment and rename to make code more clear * refine code * refine RecordBatchProjector * refine error * refine function name --------- Co-authored-by: ZENOTME <[email protected]>

ZENOTME mentioned this pull request Nov 19, 2024

feat: Add equality delete writer #372

Merged

ZENOTME force-pushed the eq_delete branch from 2d38b99 to ac29484 Compare November 19, 2024 13:26

liurenjie1024 reviewed Nov 20, 2024

View reviewed changes

ZENOTME force-pushed the eq_delete branch from 88c7ff6 to c3e7947 Compare November 20, 2024 15:01

liurenjie1024 reviewed Nov 25, 2024

View reviewed changes

liurenjie1024 reviewed Nov 26, 2024

View reviewed changes

ZENOTME force-pushed the eq_delete branch from a8be174 to e6abbc5 Compare November 26, 2024 13:39

liurenjie1024 previously approved these changes Nov 27, 2024

View reviewed changes

ZENOTME added 6 commits November 27, 2024 19:44

copy from apache#372

623e1d1

fix test and refine ArrowFieldProjector

f3bdcac

add comment and rename to make code more clear

5ebe261

refine code

ea606b6

refine RecordBatchProjector

4acbb92

refine error

11bb9b0

ZENOTME force-pushed the eq_delete branch from e6abbc5 to 11bb9b0 Compare November 27, 2024 11:45

refine function name

a5fab3a

liurenjie1024 approved these changes Nov 28, 2024

View reviewed changes

liurenjie1024 merged commit 62541fc into apache:main Nov 28, 2024
16 checks passed

ZENOTME mentioned this pull request Nov 28, 2024

Implement the equality delete writer #341

Closed

ZENOTME mentioned this pull request Dec 10, 2024

refactor: avoid async_trait macro for IcebergWriter and provide extra dyn trait for object safety #760

Open

ZENOTME deleted the eq_delete branch December 23, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add equality delete writer #703

feat: Add equality delete writer #703

ZENOTME commented Nov 19, 2024 •

edited

Loading

liurenjie1024 left a comment

liurenjie1024 Nov 20, 2024

liurenjie1024 Nov 20, 2024

ZENOTME Nov 20, 2024

liurenjie1024 Nov 25, 2024

ZENOTME Nov 25, 2024

liurenjie1024 Nov 25, 2024

liurenjie1024 Nov 25, 2024

liurenjie1024 Nov 25, 2024

liurenjie1024 Nov 25, 2024

ZENOTME Nov 25, 2024

liurenjie1024 Nov 26, 2024

liurenjie1024 Nov 26, 2024

ZENOTME Nov 26, 2024

liurenjie1024 left a comment

liurenjie1024 Nov 27, 2024

liurenjie1024 left a comment

	"Equality delete inner writer does not exist",
	"Equality delete inner writer has been closed.",

	.ok_or_else(\|\| Error::new(ErrorKind::Unexpected, "Field not found"))?;
	.ok_or_else(\|\| Error::new(ErrorKind::Unexpected, "Field not found").with_context("field_id", id)?;

feat: Add equality delete writer #703

feat: Add equality delete writer #703

Conversation

ZENOTME commented Nov 19, 2024 • edited Loading

liurenjie1024 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

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

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

ZENOTME commented Nov 19, 2024 •

edited

Loading