[PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME opened a new pull request, #704:
URL: https://github.com/apache/iceberg-rust/pull/704

   Complete #340


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["jonathanc-n (via GitHub)" <gi...@apache.org>]

jonathanc-n commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1953805959


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,
+}
+
+impl PositionDeleteInput {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: String, offsets: Vec<i64>) -> Self {
+        PositionDeleteInput { path, offsets }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput>> for PositionDeleteWriter<B> {
+    async fn write(&mut self, input: Vec<PositionDeleteInput>) -> Result<()> {
+        let mut path_column_builder = StringBuilder::new();
+        let mut offset_column_builder = PrimitiveBuilder::<Int64Type>::new();
+        for input in input.into_iter() {

Review Comment:
   Change variable here? ex. `pd_input`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1966682923


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   We can't simply this code using `#[async_trait]` because PositionDeleteInput take the reference, so in here we should convert them into RecordBatch first in sync code and then return a async function to write them. cc @liurenjie1024 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1976816015


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   Sounds reasonable to me, user is free to choose how to buffer outside.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2653718497

   Hi @liurenjie1024  @jonathanc-n. I have fixed this PR. It's ready for review. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2693595000

   hi @liurenjie1024, is there other improvement for this PR? I think it's ready to go.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2503873873

   > There are two kinds of writers in iceberg:
   > 
   > 1. Plain position delete writer: https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > 2. Sorting position delete writer:
   >    https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > 
   > It seems that this pr tries to implement 2, while there are some missing part there. I would suggest to implement 1 first as it's easier, what do you think?
   
   Is position delete must be sorted or it just be optional? From the iceberg spec, it looks like it must be sorted. https://iceberg.apache.org/spec/#position-delete-files:~:text=The%20rows%20in%20the%20delete%20file%20must%20be%20sorted%20by%20file_path%20then%20pos%20to%20optimize%20filtering%20rows%20while%20scanning.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2513388680

   I think we can resolve #741 first before this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2505139679

   > > There are two kinds of writers in iceberg:
   > > 
   > > 1. Plain position delete writer: https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > > 2. Sorting position delete writer:
   > >    https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > > 
   > > It seems that this pr tries to implement 2, while there are some missing part there. I would suggest to implement 1 first as it's easier, what do you think?
   > 
   > Is position delete must be sorted or it just be optional? From the iceberg spec, it looks like it must be sorted. https://iceberg.apache.org/spec/#position-delete-files:~:text=The%20rows%20in%20the%20delete%20file%20must%20be%20sorted%20by%20file_path%20then%20pos%20to%20optimize%20filtering%20rows%20while%20scanning.
   
   Yes, it's required in spec, but compute engine could sort this before passing to writer, and writer doesn't need to handle sorting itself.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] closed pull request #704: feat: support position delete writer
URL: https://github.com/apache/iceberg-rust/pull/704


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["jonathanc-n (via GitHub)" <gi...@apache.org>]

jonathanc-n commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2652921810

   @ZENOTME  Are you still working on this? I'm looking to work on one of the two writers


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1966554217


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,
+}
+
+impl PositionDeleteInput {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: String, offsets: Vec<i64>) -> Self {
+        PositionDeleteInput { path, offsets }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {

Review Comment:
   Do you mean that for `PositionDeleteInput`, we should buffer them and write them as a batch?🤔 E.g.
   ```
   pub struct PositionDeleteWriter {
     // path -> row_num
     buffer: HashMap<String, Vec<i64>>
   }
   ```
   For here I don't add the buffer because we will add SortPositionDeleteWriter later and it will buffer the input and sort them, so I'm not sure whether we need to add a buffer here. Or we can let it be a optional choice?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1968813999


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";

Review Comment:
   Could we organize codes like https://github.com/apache/iceberg-rust/blob/ee4e063a42304e5badbf69d074811993ac9f0391/crates/iceberg/src/spec/manifest.rs#L576 ?
   
   ```
   const DELETE_FILE_PATH: Lazy<NestedFieldRef> = ...
   const DELETE_FILE_POS: Lazy<NestedFieldRef> = ...
   ```



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   Oh, sorry, I forgot about this. Maybe we could change `PositionDeleteInput` to like this:
   ```rust
   struct PositionDeleteInput {
      path: Arc<str>,
      pos: u64
   }
   ```



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,
+}
+
+impl PositionDeleteInput {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: String, offsets: Vec<i64>) -> Self {
+        PositionDeleteInput { path, offsets }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {

Review Comment:
   Oh, sorry, I forget this is not sorting position delete writer. Please add comments like [java implementation](https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L43) here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-4001166123

   This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1860049342


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,320 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::collections::BTreeMap;
+use std::sync::Arc;
+
+use arrow_array::{ArrayRef, Int64Array, RecordBatch, StringArray};
+use arrow_schema::{Schema, SchemaRef};
+use once_cell::sync::Lazy;
+
+use crate::arrow::field_to_arrow_field;
+use crate::spec::{DataFile, NestedField, PrimitiveType, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::Result;
+
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct MemoryPositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+}
+
+impl<B: FileWriterBuilder> MemoryPositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B) -> Self {
+        Self { inner }
+    }
+}
+
+/// The config for `MemoryPositionDeleteWriter`.
+pub struct MemoryPositionDeleteWriterConfig {
+    /// The number of max rows to cache in memory.
+    pub cache_num: usize,
+    /// The partition value of the position delete file.
+    pub partition_value: Struct,
+}
+
+impl MemoryPositionDeleteWriterConfig {
+    /// Create a new `MemoryPositionDeleteWriterConfig`.
+    pub fn new(cache_num: usize, partition_value: Option<Struct>) -> Self {
+        Self {
+            cache_num,
+            partition_value: partition_value.unwrap_or(Struct::empty()),
+        }
+    }
+}
+
+static POSITION_DELETE_SCHEMA: Lazy<SchemaRef> = Lazy::new(|| {
+    Schema::new(vec![
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483546,
+            "file_path",
+            Type::Primitive(PrimitiveType::String),
+        )))
+        .unwrap(),
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483545,
+            "pos",
+            Type::Primitive(PrimitiveType::Long),
+        )))
+        .unwrap(),
+    ])
+    .into()
+});
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<PositionDeleteInput<'a>, Vec<DataFile>>
+    for MemoryPositionDeleteWriterBuilder<B>
+{
+    type R = MemoryPositionDeleteWriter<B>;
+    type C = MemoryPositionDeleteWriterConfig;
+
+    async fn build(self, config: Self::C) -> Result<Self::R> {
+        Ok(MemoryPositionDeleteWriter {
+            inner_writer_builder: self.inner.clone(),
+            cache_num: config.cache_num,
+            cache: BTreeMap::new(),
+            data_files: Vec::new(),
+            partition_value: config.partition_value,
+        })
+    }
+}
+
+/// Position delete input.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The offset of the position delete.
+    pub offset: i64,
+}
+
+/// The memory position delete writer.
+pub struct MemoryPositionDeleteWriter<B: FileWriterBuilder> {

Review Comment:
   ```suggestion
   pub struct PositionDeleteWriter<B: FileWriterBuilder> {
   ```
   I don't think we should add a `Memory` prefix here since it make people feel that we are storing everything in memory, and it applies to all structs.



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,320 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::collections::BTreeMap;
+use std::sync::Arc;
+
+use arrow_array::{ArrayRef, Int64Array, RecordBatch, StringArray};
+use arrow_schema::{Schema, SchemaRef};
+use once_cell::sync::Lazy;
+
+use crate::arrow::field_to_arrow_field;
+use crate::spec::{DataFile, NestedField, PrimitiveType, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::Result;
+
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct MemoryPositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+}
+
+impl<B: FileWriterBuilder> MemoryPositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B) -> Self {
+        Self { inner }
+    }
+}
+
+/// The config for `MemoryPositionDeleteWriter`.
+pub struct MemoryPositionDeleteWriterConfig {
+    /// The number of max rows to cache in memory.
+    pub cache_num: usize,
+    /// The partition value of the position delete file.
+    pub partition_value: Struct,
+}
+
+impl MemoryPositionDeleteWriterConfig {
+    /// Create a new `MemoryPositionDeleteWriterConfig`.
+    pub fn new(cache_num: usize, partition_value: Option<Struct>) -> Self {
+        Self {
+            cache_num,
+            partition_value: partition_value.unwrap_or(Struct::empty()),
+        }
+    }
+}
+
+static POSITION_DELETE_SCHEMA: Lazy<SchemaRef> = Lazy::new(|| {
+    Schema::new(vec![
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483546,
+            "file_path",
+            Type::Primitive(PrimitiveType::String),
+        )))
+        .unwrap(),
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483545,
+            "pos",
+            Type::Primitive(PrimitiveType::Long),
+        )))
+        .unwrap(),
+    ])
+    .into()
+});
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<PositionDeleteInput<'a>, Vec<DataFile>>
+    for MemoryPositionDeleteWriterBuilder<B>
+{
+    type R = MemoryPositionDeleteWriter<B>;
+    type C = MemoryPositionDeleteWriterConfig;
+
+    async fn build(self, config: Self::C) -> Result<Self::R> {
+        Ok(MemoryPositionDeleteWriter {
+            inner_writer_builder: self.inner.clone(),
+            cache_num: config.cache_num,
+            cache: BTreeMap::new(),
+            data_files: Vec::new(),
+            partition_value: config.partition_value,
+        })
+    }
+}
+
+/// Position delete input.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The offset of the position delete.
+    pub offset: i64,
+}
+
+/// The memory position delete writer.
+pub struct MemoryPositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer_builder: B,
+    cache_num: usize,
+    cache: BTreeMap<String, Vec<i64>>,
+    data_files: Vec<DataFile>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> MemoryPositionDeleteWriter<B> {
+    async fn write_cache_out(&mut self) -> Result<()> {
+        let mut keys = Vec::new();
+        let mut values = Vec::new();
+        let mut cache = std::mem::take(&mut self.cache);
+        for (key, offsets) in cache.iter_mut() {
+            offsets.sort();
+            let key_ref = key.as_str();
+            for offset in offsets {
+                keys.push(key_ref);
+                values.push(*offset);
+            }
+        }
+        let key_array = Arc::new(StringArray::from(keys)) as ArrayRef;
+        let value_array = Arc::new(Int64Array::from(values)) as ArrayRef;
+        let record_batch =
+            RecordBatch::try_new(POSITION_DELETE_SCHEMA.clone(), vec![key_array, value_array])?;
+        let mut writer = self.inner_writer_builder.clone().build().await?;
+        writer.write(&record_batch).await?;
+        self.data_files
+            .extend(writer.close().await?.into_iter().map(|mut res| {
+                res.content(crate::spec::DataContentType::PositionDeletes);
+                res.partition(self.partition_value.clone());
+                res.build().expect("Guaranteed to be valid")
+            }));
+        Ok(())
+    }
+}
+
+/// Implement `IcebergWriter` for `PositionDeleteWriter`.
+impl<'a, B: FileWriterBuilder> IcebergWriter<PositionDeleteInput<'a>>
+    for MemoryPositionDeleteWriter<B>
+{
+    #[doc = " Write data to iceberg table."]
+    #[must_use]
+    #[allow(clippy::type_complexity, clippy::type_repetition_in_bounds)]
+    fn write<'life0, 'async_trait>(
+        &'life0 mut self,
+        input: PositionDeleteInput<'a>,
+    ) -> ::core::pin::Pin<
+        Box<dyn ::core::future::Future<Output = Result<()>> + ::core::marker::Send + 'async_trait>,
+    >
+    where
+        'life0: 'async_trait,
+        Self: 'async_trait,

Review Comment:
   Please remove these auto generated lifetime markers and prefix of types



##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -607,6 +607,19 @@ impl SchemaVisitor for ToArrowSchemaConverter {
     }
 }
 
+/// Convert iceberg field to an arrow field.
+pub fn field_to_arrow_field(field: &crate::spec::NestedFieldRef) -> Result<FieldRef> {
+    let mut converter = ToArrowSchemaConverter;

Review Comment:
   The implementation is a little hack to me. How about just create a one field schema, and convert it using arrow schema, then get the result?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1862125870


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,320 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::collections::BTreeMap;
+use std::sync::Arc;
+
+use arrow_array::{ArrayRef, Int64Array, RecordBatch, StringArray};
+use arrow_schema::{Schema, SchemaRef};
+use once_cell::sync::Lazy;
+
+use crate::arrow::field_to_arrow_field;
+use crate::spec::{DataFile, NestedField, PrimitiveType, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::Result;
+
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct MemoryPositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+}
+
+impl<B: FileWriterBuilder> MemoryPositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B) -> Self {
+        Self { inner }
+    }
+}
+
+/// The config for `MemoryPositionDeleteWriter`.
+pub struct MemoryPositionDeleteWriterConfig {
+    /// The number of max rows to cache in memory.
+    pub cache_num: usize,
+    /// The partition value of the position delete file.
+    pub partition_value: Struct,
+}
+
+impl MemoryPositionDeleteWriterConfig {
+    /// Create a new `MemoryPositionDeleteWriterConfig`.
+    pub fn new(cache_num: usize, partition_value: Option<Struct>) -> Self {
+        Self {
+            cache_num,
+            partition_value: partition_value.unwrap_or(Struct::empty()),
+        }
+    }
+}
+
+static POSITION_DELETE_SCHEMA: Lazy<SchemaRef> = Lazy::new(|| {
+    Schema::new(vec![
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483546,
+            "file_path",
+            Type::Primitive(PrimitiveType::String),
+        )))
+        .unwrap(),
+        field_to_arrow_field(&Arc::new(NestedField::required(
+            2147483545,
+            "pos",
+            Type::Primitive(PrimitiveType::Long),
+        )))
+        .unwrap(),
+    ])
+    .into()
+});
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<PositionDeleteInput<'a>, Vec<DataFile>>
+    for MemoryPositionDeleteWriterBuilder<B>
+{
+    type R = MemoryPositionDeleteWriter<B>;
+    type C = MemoryPositionDeleteWriterConfig;
+
+    async fn build(self, config: Self::C) -> Result<Self::R> {
+        Ok(MemoryPositionDeleteWriter {
+            inner_writer_builder: self.inner.clone(),
+            cache_num: config.cache_num,
+            cache: BTreeMap::new(),
+            data_files: Vec::new(),
+            partition_value: config.partition_value,
+        })
+    }
+}
+
+/// Position delete input.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The offset of the position delete.
+    pub offset: i64,
+}
+
+/// The memory position delete writer.
+pub struct MemoryPositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer_builder: B,
+    cache_num: usize,
+    cache: BTreeMap<String, Vec<i64>>,
+    data_files: Vec<DataFile>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> MemoryPositionDeleteWriter<B> {
+    async fn write_cache_out(&mut self) -> Result<()> {
+        let mut keys = Vec::new();
+        let mut values = Vec::new();
+        let mut cache = std::mem::take(&mut self.cache);
+        for (key, offsets) in cache.iter_mut() {
+            offsets.sort();
+            let key_ref = key.as_str();
+            for offset in offsets {
+                keys.push(key_ref);
+                values.push(*offset);
+            }
+        }
+        let key_array = Arc::new(StringArray::from(keys)) as ArrayRef;
+        let value_array = Arc::new(Int64Array::from(values)) as ArrayRef;
+        let record_batch =
+            RecordBatch::try_new(POSITION_DELETE_SCHEMA.clone(), vec![key_array, value_array])?;
+        let mut writer = self.inner_writer_builder.clone().build().await?;
+        writer.write(&record_batch).await?;
+        self.data_files
+            .extend(writer.close().await?.into_iter().map(|mut res| {
+                res.content(crate::spec::DataContentType::PositionDeletes);
+                res.partition(self.partition_value.clone());
+                res.build().expect("Guaranteed to be valid")
+            }));
+        Ok(())
+    }
+}
+
+/// Implement `IcebergWriter` for `PositionDeleteWriter`.
+impl<'a, B: FileWriterBuilder> IcebergWriter<PositionDeleteInput<'a>>
+    for MemoryPositionDeleteWriter<B>
+{
+    #[doc = " Write data to iceberg table."]
+    #[must_use]
+    #[allow(clippy::type_complexity, clippy::type_repetition_in_bounds)]
+    fn write<'life0, 'async_trait>(
+        &'life0 mut self,
+        input: PositionDeleteInput<'a>,
+    ) -> ::core::pin::Pin<
+        Box<dyn ::core::future::Future<Output = Result<()>> + ::core::marker::Send + 'async_trait>,
+    >
+    where
+        'life0: 'async_trait,
+        Self: 'async_trait,

Review Comment:
   For here we use a sync version so that seems we need to explicitly declare these auto-generated lifetime.
   The reason we need the sync version is that the input takes the reference like: `struct PositionDeleteInput<'a> `, we  need to explicitly convert it into a record batch in the sync function part and then return a async future to write this record batch.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2653556089

   > @ZENOTME Are you still working on this? I'm looking to work on one of the two writers
   
   Sorry for the late, I will work on this later. Would you like to work on sorting position delete writer after this PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1972829889


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   How about using the interface to hide containers used like the following? 
   
   ```
   struct PositionDeleteVec {
      path: String,
      pos: Vec<usize>
   }
   impl PositionDeleteVec {
     pub fn new(path: String, pos: into Iterator<usize>) -> Self {}
   }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["manuzhang (via GitHub)" <gi...@apache.org>]

manuzhang commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-3478063714

   @liurenjie1024 @Xuanwo Shall we merge this PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["jonathanc-n (via GitHub)" <gi...@apache.org>]

jonathanc-n commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1953805959


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,
+}
+
+impl PositionDeleteInput {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: String, offsets: Vec<i64>) -> Self {
+        PositionDeleteInput { path, offsets }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput>> for PositionDeleteWriter<B> {
+    async fn write(&mut self, input: Vec<PositionDeleteInput>) -> Result<()> {
+        let mut path_column_builder = StringBuilder::new();
+        let mut offset_column_builder = PrimitiveBuilder::<Int64Type>::new();
+        for input in input.into_iter() {

Review Comment:
   Change variable here? ex. `pd_input` 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1968910011


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   Does this cause too much overhead for large input? We need to call clone for each row.🤔



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1972684368


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,277 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::future::Future;
+use std::pin::Pin;
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+const POS_DELETE_FIELD1_NAME: &str = "file_path";
+const POS_DELETE_FIELD1_ID: i32 = 2147483546;
+const POS_DELETE_FIELD2_NAME: &str = "pos";
+const POS_DELETE_FIELD2_ID: i32 = 2147483545;
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD1_ID,
+                POS_DELETE_FIELD1_NAME,
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                POS_DELETE_FIELD2_ID,
+                POS_DELETE_FIELD2_NAME,
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput<'a> {
+    /// The path of the file.
+    pub path: &'a str,
+    /// The row number in data file
+    pub pos: i64,
+}
+
+impl<'a> PositionDeleteInput<'a> {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: &'a str, row: i64) -> Self {
+        Self { path, pos: row }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<'a, B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput<'a>>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {
+    inner_writer: Option<B::R>,
+    partition_value: Struct,
+}
+
+impl<'a, B: FileWriterBuilder> IcebergWriter<Vec<PositionDeleteInput<'a>>>

Review Comment:
   Yes, that's a trade off. But this approach makes this trait difficult to read and maintain.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on code in PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#discussion_r1962828050


##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,

Review Comment:
   ```suggestion
       /// The row number in data file..
       pub row: i64,
   ```



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",

Review Comment:
   Please make these constants.



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",

Review Comment:
   Ditto



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,

Review Comment:
   We should not ask user to think about the container.



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,

Review Comment:
   ```suggestion
       pub path: &'a str,
   ```



##########
crates/iceberg/src/writer/base_writer/position_delete_file_writer.rs:
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Position delete file writer.
+use std::sync::Arc;
+
+use arrow_array::builder::{PrimitiveBuilder, StringBuilder};
+use arrow_array::types::Int64Type;
+use arrow_array::RecordBatch;
+use once_cell::sync::Lazy;
+
+use crate::arrow::schema_to_arrow_schema;
+use crate::spec::{DataContentType, DataFile, NestedField, PrimitiveType, Schema, Struct, Type};
+use crate::writer::file_writer::{FileWriter, FileWriterBuilder};
+use crate::writer::{IcebergWriter, IcebergWriterBuilder};
+use crate::{Error, ErrorKind, Result};
+
+static POSITION_DELETE_SCHEMA: Lazy<Schema> = Lazy::new(|| {
+    Schema::builder()
+        .with_fields(vec![
+            Arc::new(NestedField::required(
+                2147483546,
+                "file_path",
+                Type::Primitive(PrimitiveType::String),
+            )),
+            Arc::new(NestedField::required(
+                2147483545,
+                "pos",
+                Type::Primitive(PrimitiveType::Long),
+            )),
+        ])
+        .build()
+        .unwrap()
+});
+
+/// Position delete input.
+#[derive(Clone, PartialEq, Eq, Ord, PartialOrd, Debug)]
+pub struct PositionDeleteInput {
+    /// The path of the file.
+    pub path: String,
+    /// The offset of the position delete.
+    pub offsets: Vec<i64>,
+}
+
+impl PositionDeleteInput {
+    /// Create a new `PositionDeleteInput`.
+    pub fn new(path: String, offsets: Vec<i64>) -> Self {
+        PositionDeleteInput { path, offsets }
+    }
+}
+/// Builder for `MemoryPositionDeleteWriter`.
+#[derive(Clone)]
+pub struct PositionDeleteWriterBuilder<B: FileWriterBuilder> {
+    inner: B,
+    partition_value: Option<Struct>,
+}
+
+impl<B: FileWriterBuilder> PositionDeleteWriterBuilder<B> {
+    /// Create a new `MemoryPositionDeleteWriterBuilder` using a `FileWriterBuilder`.
+    pub fn new(inner: B, partition_value: Option<Struct>) -> Self {
+        Self {
+            inner,
+            partition_value,
+        }
+    }
+}
+
+#[async_trait::async_trait]
+impl<B: FileWriterBuilder> IcebergWriterBuilder<Vec<PositionDeleteInput>>
+    for PositionDeleteWriterBuilder<B>
+{
+    type R = PositionDeleteWriter<B>;
+
+    async fn build(self) -> Result<Self::R> {
+        Ok(PositionDeleteWriter {
+            inner_writer: Some(self.inner.build().await?),
+            partition_value: self.partition_value.unwrap_or(Struct::empty()),
+        })
+    }
+}
+
+/// Position delete writer.
+pub struct PositionDeleteWriter<B: FileWriterBuilder> {

Review Comment:
   We should buffer in memory about for the input row number. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["liurenjie1024 (via GitHub)" <gi...@apache.org>]

liurenjie1024 commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-3479764135

   > @liurenjie1024 @Xuanwo Any more comments? Shall we merge this PR?
   
   It has been a long time and I need to take a review again.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] feat: support position delete writer [iceberg-rust]

["ZENOTME (via GitHub)" <gi...@apache.org>]

ZENOTME commented on PR #704:
URL: https://github.com/apache/iceberg-rust/pull/704#issuecomment-2505206247

   > > > There are two kinds of writers in iceberg:
   > > > 
   > > > 1. Plain position delete writer: https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > > > 2. Sorting position delete writer:
   > > >    https://github.com/apache/iceberg/blob/da2ad389fd9ba8222f6fb3f57922209c239a7045/core/src/main/java/org/apache/iceberg/deletes/PositionDeleteWriter.java#L49
   > > > 
   > > > It seems that this pr tries to implement 2, while there are some missing part there. I would suggest to implement 1 first as it's easier, what do you think?
   > > 
   > > 
   > > Is position delete must be sorted or it just be optional? From the iceberg spec, it looks like it must be sorted. https://iceberg.apache.org/spec/#position-delete-files:~:text=The%20rows%20in%20the%20delete%20file%20must%20be%20sorted%20by%20file_path%20then%20pos%20to%20optimize%20filtering%20rows%20while%20scanning.
   > 
   > Yes, it's required in spec, but some compute engine could sort this before passing to writer, and writer doesn't need to handle sorting itself.
   
   Make sense. Let's implement 1 first


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org