From efb7e7731d2eeacbb5bf60da81fb412fc49bdfb3 Mon Sep 17 00:00:00 2001
From: Jimmy Lu <jimmylu@meta.com>
Date: Wed, 3 Apr 2024 14:30:19 -0700
Subject: [PATCH] Allow LocalWriteFile::size to be called after close (#9350)

Summary:
Pull Request resolved: https://github.com/facebookincubator/velox/pull/9350

Alpha writer is accessing `size` after the file close.  The only implementation that does not allow this is `LocalWriteFile`.  Fix this in `LocalWriteFile` so this is less likely to causing problem on call sites.

Reviewed By: xiaoxmeng

Differential Revision: D55662422

fbshipit-source-id: a487a0573a72b2715cc72d5da1663204fa3be062
---
 velox/common/file/File.cpp           |  5 ++---
 velox/common/file/File.h             | 11 ++++++++---
 velox/common/file/tests/FileTest.cpp |  2 ++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/velox/common/file/File.cpp b/velox/common/file/File.cpp
index 6547750de5f0..ea5ef04b4b7e 100644
--- a/velox/common/file/File.cpp
+++ b/velox/common/file/File.cpp
@@ -273,6 +273,7 @@ void LocalWriteFile::append(std::string_view data) {
       bytesWritten,
       data.size(),
       folly::errnoStr(errno));
+  size_ += bytesWritten;
 }
 
 void LocalWriteFile::append(std::unique_ptr<folly::IOBuf> data) {
@@ -298,6 +299,7 @@ void LocalWriteFile::append(std::unique_ptr<folly::IOBuf> data) {
       "Failure in LocalWriteFile::append, {} vs {}",
       totalBytesWritten,
       totalBytesToWrite);
+  size_ += totalBytesWritten;
 }
 
 void LocalWriteFile::flush() {
@@ -322,7 +324,4 @@ void LocalWriteFile::close() {
   }
 }
 
-uint64_t LocalWriteFile::size() const {
-  return ftell(file_);
-}
 } // namespace facebook::velox
diff --git a/velox/common/file/File.h b/velox/common/file/File.h
index acd1359f1e08..2ce64cad3b98 100644
--- a/velox/common/file/File.h
+++ b/velox/common/file/File.h
@@ -155,7 +155,9 @@ class WriteFile {
   // Close the file. Any cleanup (disk flush, etc.) will be done here.
   virtual void close() = 0;
 
-  // Current file size, i.e. the sum of all previous Appends.
+  /// Current file size, i.e. the sum of all previous Appends.  No flush should
+  /// be needed to get the exact size written, and this should be able to be
+  /// called after the file close.
   virtual uint64_t size() const = 0;
 };
 
@@ -283,11 +285,14 @@ class LocalWriteFile final : public WriteFile {
   void append(std::unique_ptr<folly::IOBuf> data) final;
   void flush() final;
   void close() final;
-  uint64_t size() const final;
+
+  uint64_t size() const final {
+    return size_;
+  }
 
  private:
   FILE* file_;
-  mutable long size_;
+  uint64_t size_{0};
   bool closed_{false};
 };
 
diff --git a/velox/common/file/tests/FileTest.cpp b/velox/common/file/tests/FileTest.cpp
index af5654a72369..9f837fc94a23 100644
--- a/velox/common/file/tests/FileTest.cpp
+++ b/velox/common/file/tests/FileTest.cpp
@@ -134,6 +134,8 @@ TEST(LocalFile, writeAndRead) {
     {
       LocalWriteFile writeFile(filename);
       writeData(&writeFile, useIOBuf);
+      writeFile.close();
+      ASSERT_EQ(writeFile.size(), 15 + kOneMB);
     }
     LocalReadFile readFile(filename);
     readData(&readFile);