xanderbailey commented on code in PR #3236:
URL: https://github.com/apache/iceberg-rust/pull/3236#discussion_r4021414755


##########
crates/storage/opendal/src/lib.rs:
##########
@@ -635,21 +635,56 @@ impl FileRead for OpenDalReader {
 }
 
 /// Wrapper around `opendal::Writer` that implements `FileWrite`.
-pub(crate) struct OpenDalWriter(pub(crate) opendal::Writer);
+pub(crate) struct OpenDalWriter {
+    inner: opendal::Writer,
+    bytes_written: u64,
+}
+
+impl OpenDalWriter {
+    pub(crate) fn new(inner: opendal::Writer) -> Self {
+        Self {
+            inner,
+            bytes_written: 0,
+        }
+    }
+}
 
 #[async_trait]
 impl FileWrite for OpenDalWriter {
     async fn write(&mut self, bs: Bytes) -> Result<()> {
-        Ok(opendal::Writer::write(&mut self.0, bs)
+        let len = bs.len() as u64;
+        opendal::Writer::write(&mut self.inner, bs)
             .await
-            .map_err(from_opendal_error)?)
+            .map_err(from_opendal_error)?;
+        self.bytes_written += len;
+        Ok(())
     }
 
-    async fn close(&mut self) -> Result<()> {
-        let _ = opendal::Writer::close(&mut self.0)
+    async fn close(&mut self) -> Result<FileMetadata> {
+        let metadata = opendal::Writer::close(&mut self.inner)
             .await
             .map_err(from_opendal_error)?;
-        Ok(())
+
+        // `Metadata::content_length()` silently returns 0 when the service 
did not report a
+        // size, and most object stores don't: S3 only populates it from the 
`x-amz-object-size`
+        // response header, which general-purpose buckets never send. A bogus 
0 here would be
+        // written into `manifest_length` and into the AGS1 `file_length` used 
for truncation
+        // protection, making the file permanently unreadable, so trust our 
own byte count and
+        // only use the service value to detect a genuine mismatch.

Review Comment:
   I will admit this was not my find, claude found that S3 openDal will return 
0 here so we need to keep track of the bytes written ourselves.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to