Skip to content

Commit 3ca59f6

Browse files
zhjwpkuzhjwpku
committed
fix review comments
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
1 parent 5894d3e commit 3ca59f6

File tree

8 files changed

+41
-43
lines changed

8 files changed

+41
-43
lines changed

‎src/iceberg/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ install(FILES ${CMAKE_CURRENT_BINARY_DIR}/iceberg_export.h
5656
DESTINATION ${ICEBERG_INSTALL_INCLUDEDIR}/iceberg)
5757

5858
if(ICEBERG_BUILD_BUNDLE)
59-
set(ICEBERG_BUNDLE_SOURCES arrow/demo_arrow.cc arrow/io/local_file_io.cc
59+
set(ICEBERG_BUNDLE_SOURCES arrow/demo_arrow.cc arrow/io/arrow_fs_file_io.cc
6060
avro/demo_avro.cc)
6161

6262
# Libraries to link with exported libiceberg_bundle.{so,a}.

‎src/iceberg/arrow/arrow_error_transform_internal.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727

2828
namespace iceberg::arrow::internal {
2929

30+
namespace {
31+
3032
inline ErrorKind ToErrorKind(const ::arrow::Status& status) {
3133
switch (status.code()) {
3234
case ::arrow::StatusCode::IOError:
@@ -44,6 +46,8 @@ inline ErrorKind ToErrorKind(const ::arrow::Status& status) {
4446
} \
4547
lhs = std::move(result_name).ValueOrDie();
4648

49+
} // namespace
50+
4751
#define ICEBERG_INTERNAL_ASSIGN_OR_RETURN(lhs, rexpr) \
4852
ICEBERG_INTERNAL_ASSIGN_OR_RETURN_IMPL( \
4953
ARROW_ASSIGN_OR_RAISE_NAME(_error_or_value, __COUNTER__), lhs, rexpr, \

src/iceberg/arrow/io/local_file_io.cc renamed to src/iceberg/arrow/io/arrow_fs_file_io.cc

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* under the License.
1818
*/
1919

20-
#include "iceberg/arrow/io/local_file_io.h"
20+
#include "iceberg/arrow/io/arrow_fs_file_io.h"
2121

2222
#include <filesystem>
2323

@@ -28,14 +28,14 @@
2828
namespace iceberg::arrow::io {
2929

3030
/// \brief Read the content of the file at the given location.
31-
expected<std::string, Error> LocalFileIO::ReadFile(const std::string& file_location,
32-
std::optional<size_t> length) {
31+
expected<std::string, Error> ArrowFileSystemFileIO::ReadFile(
32+
const std::string& file_location, std::optional<size_t> length) {
3333
// We don't support reading a file with a specific length.
3434
if (length.has_value()) {
3535
return unexpected(Error(ErrorKind::kInvalidArgument, "Length is not supported"));
3636
}
3737
std::string content;
38-
ICEBERG_INTERNAL_ASSIGN_OR_RETURN(auto file, local_fs_->OpenInputFile(file_location));
38+
ICEBERG_INTERNAL_ASSIGN_OR_RETURN(auto file, arrow_fs_->OpenInputFile(file_location));
3939
ICEBERG_INTERNAL_ASSIGN_OR_RETURN(auto file_size, file->GetSize());
4040

4141
content.resize(file_size);
@@ -47,31 +47,28 @@ expected<std::string, Error> LocalFileIO::ReadFile(const std::string& file_locat
4747
}
4848

4949
/// \brief Write the given content to the file at the given location.
50-
expected<void, Error> LocalFileIO::WriteFile(const std::string& file_location,
51-
std::string_view content, bool overwrite) {
52-
if (!overwrite && FileExists(file_location)) {
53-
return unexpected(
54-
Error(ErrorKind::kAlreadyExists, std::format("File {} exists", file_location)));
55-
}
50+
expected<void, Error> ArrowFileSystemFileIO::WriteFile(const std::string& file_location,
51+
std::string_view content,
52+
bool overwrite) {
53+
// auto file_info = arrow_fs_->GetFileInfo(file_location);
54+
// if (file_info.status().ok() && !overwrite) {
55+
// return unexpected(
56+
// Error(ErrorKind::kAlreadyExists, std::format("File {} exists",
57+
// file_location)));
58+
// }
5659
ICEBERG_INTERNAL_ASSIGN_OR_RETURN(auto file,
57-
local_fs_->OpenOutputStream(file_location));
60+
arrow_fs_->OpenOutputStream(file_location));
5861
ICEBERG_INTERNAL_RETURN_NOT_OK(file->Write(content.data(), content.size()));
62+
ICEBERG_INTERNAL_RETURN_NOT_OK(file->Flush());
63+
ICEBERG_INTERNAL_RETURN_NOT_OK(file->Close());
5964
return {};
6065
}
6166

6267
/// \brief Delete a file at the given location.
63-
expected<void, Error> LocalFileIO::DeleteFile(const std::string& file_location) {
64-
if (!FileExists(file_location)) {
65-
return unexpected(Error(ErrorKind::kNoSuchFile,
66-
std::format("File {} does not exist", file_location)));
67-
}
68-
ICEBERG_INTERNAL_RETURN_NOT_OK(local_fs_->DeleteFile(file_location));
68+
expected<void, Error> ArrowFileSystemFileIO::DeleteFile(
69+
const std::string& file_location) {
70+
ICEBERG_INTERNAL_RETURN_NOT_OK(arrow_fs_->DeleteFile(file_location));
6971
return {};
7072
}
7173

72-
bool LocalFileIO::FileExists(const std::string& location) {
73-
// ::arrow::fs::LocalFileSystem does not have a exists method.
74-
return std::filesystem::exists(location);
75-
}
76-
7774
} // namespace iceberg::arrow::io

src/iceberg/arrow/io/local_file_io.h renamed to src/iceberg/arrow/io/arrow_fs_file_io.h

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,20 @@
2121

2222
#include <memory>
2323

24-
#include <arrow/filesystem/localfs.h>
24+
#include <arrow/filesystem/filesystem.h>
2525

2626
#include "iceberg/file_io.h"
2727
#include "iceberg/iceberg_bundle_export.h"
2828

2929
namespace iceberg::arrow::io {
3030

31-
/// \brief A concrete implementation of FileIO for file system.
32-
class ICEBERG_BUNDLE_EXPORT LocalFileIO : public FileIO {
31+
/// \brief A concrete implementation of FileIO for Arrow file system.
32+
class ICEBERG_BUNDLE_EXPORT ArrowFileSystemFileIO : public FileIO {
3333
public:
34-
explicit LocalFileIO(std::shared_ptr<::arrow::fs::LocalFileSystem>& local_fs)
35-
: local_fs_(local_fs) {}
34+
explicit ArrowFileSystemFileIO(std::shared_ptr<::arrow::fs::FileSystem> arrow_fs)
35+
: arrow_fs_(std::move(arrow_fs)) {}
3636

37-
~LocalFileIO() override = default;
37+
~ArrowFileSystemFileIO() override = default;
3838

3939
/// \brief Read the content of the file at the given location.
4040
expected<std::string, Error> ReadFile(const std::string& file_location,
@@ -48,10 +48,7 @@ class ICEBERG_BUNDLE_EXPORT LocalFileIO : public FileIO {
4848
expected<void, Error> DeleteFile(const std::string& file_location) override;
4949

5050
private:
51-
/// \brief Check if a file exists
52-
bool FileExists(const std::string& location);
53-
54-
std::shared_ptr<::arrow::fs::LocalFileSystem>& local_fs_;
51+
std::shared_ptr<::arrow::fs::FileSystem> arrow_fs_;
5552
};
5653

5754
} // namespace iceberg::arrow::io

‎src/iceberg/error.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ namespace iceberg {
3030
enum class ErrorKind {
3131
kNoSuchNamespace,
3232
kAlreadyExists,
33-
kNoSuchFile,
3433
kNoSuchTable,
3534
kCommitStateUnknown,
3635
kInvalidArgument,

‎src/iceberg/file_io.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929

3030
namespace iceberg {
3131

32-
/// \brief Pluggable module for reading, writing, and deleting metadata files.
32+
/// \brief Pluggable module for reading, writing, and deleting files.
3333
///
3434
/// This module only handle metadata files, not data files. The metadata files
3535
/// are typically small and are used to store schema, partition information,

‎test/CMakeLists.txt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ if(ICEBERG_BUILD_BUNDLE)
4444
target_link_libraries(arrow_test PRIVATE iceberg_bundle_static GTest::gtest_main)
4545
add_test(NAME arrow_test COMMAND arrow_test)
4646

47-
add_executable(local_file_io_test)
48-
target_sources(local_file_io_test PRIVATE local_file_io_test.cc)
49-
target_link_libraries(local_file_io_test PRIVATE iceberg_bundle_static
50-
GTest::gtest_main)
51-
add_test(NAME local_file_io_test COMMAND local_file_io_test)
47+
add_executable(arrow_fs_file_io_test)
48+
target_sources(arrow_fs_file_io_test PRIVATE arrow_fs_file_io_test.cc)
49+
target_link_libraries(arrow_fs_file_io_test PRIVATE iceberg_bundle_static
50+
GTest::gtest_main)
51+
add_test(NAME arrow_fs_file_io_test COMMAND arrow_fs_file_io_test)
5252
endif()

test/local_file_io_test.cc renamed to test/arrow_fs_file_io_test.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,18 @@
1717
* under the License.
1818
*/
1919

20-
#include "iceberg/arrow/io/local_file_io.h"
20+
#include "iceberg/arrow/io/arrow_fs_file_io.h"
2121

2222
#include <filesystem>
2323

24+
#include <arrow/filesystem/localfs.h>
2425
#include <gtest/gtest.h>
2526

2627
class LocalFileIOTest : public testing::Test {
2728
protected:
2829
void SetUp() override {
2930
local_fs_ = std::make_shared<arrow::fs::LocalFileSystem>();
30-
file_io_ = std::make_shared<iceberg::arrow::io::LocalFileIO>(local_fs_);
31+
file_io_ = std::make_shared<iceberg::arrow::io::ArrowFileSystemFileIO>(local_fs_);
3132
}
3233

3334
std::shared_ptr<arrow::fs::LocalFileSystem> local_fs_;
@@ -54,5 +55,5 @@ TEST_F(LocalFileIOTest, DeleteFile) {
5455
EXPECT_TRUE(del_res.has_value());
5556

5657
del_res = file_io_->DeleteFile(tmpfile.string());
57-
EXPECT_EQ(del_res.error().kind, iceberg::ErrorKind::kNoSuchFile);
58+
EXPECT_EQ(del_res.error().kind, iceberg::ErrorKind::kIOError);
5859
}

0 commit comments

Comments
 (0)