fix some review comments

Author: HuaHuaY

src/iceberg/parquet/parquet_reader.cc

@@ -237,12 +237,12 @@ class ParquetReader::Impl {
   std::shared_ptr<::iceberg::Schema> read_schema_;
   // The projection result to apply to the read schema.
   SchemaProjection projection_;
+  // The input stream to read Parquet file.
+  std::shared_ptr<::arrow::io::RandomAccessFile> input_stream_;
   // Parquet file reader to create RecordBatchReader.
   std::unique_ptr<::parquet::arrow::FileReader> reader_;
   // The context to keep track of the reading progress.
   std::unique_ptr<ReadContext> context_;
-  // The input stream to read Parquet file.
-  std::shared_ptr<::arrow::io::RandomAccessFile> input_stream_;
 };
 
 ParquetReader::~ParquetReader() = default;

src/iceberg/parquet/parquet_writer.cc

@@ -66,12 +66,9 @@ class ParquetWriter::Impl {
     auto schema_node = std::static_pointer_cast<::parquet::schema::GroupNode>(
         schema_descriptor->schema_root());
 
-    std::shared_ptr<::arrow::KeyValueMetadata> metadata =
-        ::arrow::key_value_metadata(options.properties);
-
     ICEBERG_ASSIGN_OR_RAISE(output_stream_, OpenOutputStream(options));
     auto file_writer = ::parquet::ParquetFileWriter::Open(output_stream_, schema_node,
-                                                          writer_properties, metadata);
+                                                          writer_properties);
     ICEBERG_ARROW_RETURN_NOT_OK(::parquet::arrow::FileWriter::Make(
         pool_, std::move(file_writer), arrow_schema_, arrow_writer_properties, &writer_));
 
@@ -106,10 +103,10 @@ class ParquetWriter::Impl {
   ::arrow::MemoryPool* pool_ = ::arrow::default_memory_pool();
   // Schema to write from the Parquet file.
   std::shared_ptr<::arrow::Schema> arrow_schema_;
-  // Parquet file writer to write ArrowArray.
-  std::unique_ptr<::parquet::arrow::FileWriter> writer_;
   // The output stream to write Parquet file.
   std::shared_ptr<::arrow::io::OutputStream> output_stream_;
+  // Parquet file writer to write ArrowArray.
+  std::unique_ptr<::parquet::arrow::FileWriter> writer_;
 };
 
 ParquetWriter::~ParquetWriter() = default;

src/iceberg/schema_internal.cc

@@ -24,6 +24,7 @@
 #include <optional>
 #include <string>
 
+#include "iceberg/constants.h"
 #include "iceberg/schema.h"
 #include "iceberg/type.h"
 #include "iceberg/util/macros.h"
@@ -45,7 +46,7 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n
   NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderInit(&metadata_buffer, nullptr));
   if (field_id.has_value()) {
     NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderAppend(
-        &metadata_buffer, ArrowCharView(std::string(kFieldIdKey).c_str()),
+        &metadata_buffer, ArrowCharView(std::string(kParquetFieldIdKey).c_str()),
         ArrowCharView(std::to_string(field_id.value()).c_str())));
   }
 
@@ -185,8 +186,8 @@ int32_t GetFieldId(const ArrowSchema& schema) {
     return kUnknownFieldId;
   }
 
-  ArrowStringView field_id_key{.data = kFieldIdKey.data(),
-                               .size_bytes = kFieldIdKey.size()};
+  ArrowStringView field_id_key{.data = kParquetFieldIdKey.data(),
+                               .size_bytes = kParquetFieldIdKey.size()};
   ArrowStringView field_id_value;
   if (ArrowMetadataGetValue(schema.metadata, field_id_key, &field_id_value) !=
       NANOARROW_OK) {

src/iceberg/schema_internal.h

@@ -24,18 +24,12 @@
 
 #include <nanoarrow/nanoarrow.h>
 
-#include "iceberg/constants.h"
 #include "iceberg/iceberg_export.h"
 #include "iceberg/result.h"
 #include "iceberg/type_fwd.h"
 
 namespace iceberg {
 
-// Apache Arrow C++ uses "PARQUET:field_id" to store field IDs for Parquet.
-// Here we follow a similar convention for Iceberg but we might also add
-// "PARQUET:field_id" in the future once we implement a Parquet writer.
-constexpr std::string_view kFieldIdKey = kParquetFieldIdKey;
-
 /// \brief Convert an Iceberg schema to an Arrow schema.
 ///
 /// \param[in] schema The Iceberg schema to convert.

test/arrow_test.cc

@@ -28,6 +28,7 @@
 #include <arrow/util/key_value_metadata.h>
 #include <gtest/gtest.h>
 
+#include "iceberg/constants.h"
 #include "iceberg/schema.h"
 #include "iceberg/schema_internal.h"
 #include "matchers.h"
@@ -64,8 +65,8 @@ TEST_P(ToArrowSchemaTest, PrimitiveType) {
   ASSERT_TRUE(field->type()->Equals(param.arrow_type));
 
   auto metadata = field->metadata();
-  ASSERT_TRUE(metadata->Contains(kFieldIdKey));
-  ASSERT_EQ(metadata->Get(kFieldIdKey), std::to_string(kFieldId));
+  ASSERT_TRUE(metadata->Contains(kParquetFieldIdKey));
+  ASSERT_EQ(metadata->Get(kParquetFieldIdKey), std::to_string(kFieldId));
 }
 
 INSTANTIATE_TEST_SUITE_P(
@@ -112,8 +113,8 @@ void CheckArrowField(const ::arrow::Field& field, ::arrow::Type::type type_id,
 
   auto metadata = field.metadata();
   ASSERT_TRUE(metadata != nullptr);
-  ASSERT_TRUE(metadata->Contains(kFieldIdKey));
-  ASSERT_EQ(metadata->Get(kFieldIdKey), std::to_string(field_id));
+  ASSERT_TRUE(metadata->Contains(kParquetFieldIdKey));
+  ASSERT_EQ(metadata->Get(kParquetFieldIdKey), std::to_string(field_id));
 }
 
 }  // namespace
@@ -241,7 +242,7 @@ TEST_P(FromArrowSchemaTest, PrimitiveType) {
 
   auto metadata =
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kFieldId)}});
+          {std::string(kParquetFieldIdKey), std::to_string(kFieldId)}});
   auto arrow_schema = ::arrow::schema({::arrow::field(
       std::string(kFieldName), param.arrow_type, param.optional, std::move(metadata))});
   ArrowSchema exported_schema;
@@ -309,16 +310,16 @@ TEST(FromArrowSchemaTest, StructType) {
   auto int_field = ::arrow::field(
       std::string(kIntFieldName), ::arrow::int32(), /*nullable=*/false,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kIntFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kIntFieldId)}}));
   auto str_field = ::arrow::field(
       std::string(kStrFieldName), ::arrow::utf8(), /*nullable=*/true,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kStrFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kStrFieldId)}}));
   auto struct_type = ::arrow::struct_({int_field, str_field});
   auto struct_field = ::arrow::field(
       std::string(kStructFieldName), struct_type, /*nullable=*/false,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kStructFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kStructFieldId)}}));
   auto arrow_schema = ::arrow::schema({struct_field});
   ArrowSchema exported_schema;
   ASSERT_TRUE(::arrow::ExportSchema(*arrow_schema, &exported_schema).ok());
@@ -363,12 +364,12 @@ TEST(FromArrowSchemaTest, ListType) {
   auto element_field = ::arrow::field(
       std::string(kElemFieldName), ::arrow::int64(), /*nullable=*/true,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kElemFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kElemFieldId)}}));
   auto list_type = ::arrow::list(element_field);
   auto list_field = ::arrow::field(
       std::string(kListFieldName), list_type, /*nullable=*/false,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kListFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kListFieldId)}}));
   auto arrow_schema = ::arrow::schema({list_field});
 
   ArrowSchema exported_schema;
@@ -410,16 +411,16 @@ TEST(FromArrowSchemaTest, MapType) {
   auto key_field = ::arrow::field(
       std::string(kKeyFieldName), ::arrow::utf8(), /*nullable=*/false,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kKeyFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kKeyFieldId)}}));
   auto value_field = ::arrow::field(
       std::string(kValueFieldName), ::arrow::int32(), /*nullable=*/true,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kValueFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kValueFieldId)}}));
   auto map_type = std::make_shared<::arrow::MapType>(key_field, value_field);
   auto map_field = ::arrow::field(
       std::string(kMapFieldName), map_type, /*nullable=*/true,
       ::arrow::key_value_metadata(std::unordered_map<std::string, std::string>{
-          {std::string(kFieldIdKey), std::to_string(kFieldId)}}));
+          {std::string(kParquetFieldIdKey), std::to_string(kFieldId)}}));
   auto arrow_schema = ::arrow::schema({map_field});
 
   ArrowSchema exported_schema;

test/parquet_test.cc

@@ -26,20 +26,19 @@
 #include <arrow/table.h>
 #include <arrow/type.h>
 #include <arrow/util/key_value_metadata.h>
-#include <iceberg/arrow_c_data_guard_internal.h>
-#include <iceberg/file_reader.h>
-#include <iceberg/file_writer.h>
-#include <iceberg/result.h>
-#include <iceberg/schema_field.h>
-#include <iceberg/schema_internal.h>
 #include <parquet/arrow/reader.h>
 #include <parquet/arrow/writer.h>
 #include <parquet/metadata.h>
 
 #include "iceberg/arrow/arrow_error_transform_internal.h"
 #include "iceberg/arrow/arrow_fs_file_io_internal.h"
+#include "iceberg/file_reader.h"
+#include "iceberg/file_writer.h"
 #include "iceberg/parquet/parquet_register.h"
+#include "iceberg/result.h"
 #include "iceberg/schema.h"
+#include "iceberg/schema_field.h"
+#include "iceberg/schema_internal.h"
 #include "iceberg/type.h"
 #include "iceberg/util/checked_cast.h"
 #include "iceberg/util/macros.h"
@@ -51,15 +50,11 @@ namespace {
 
 Status WriteTable(std::shared_ptr<::arrow::Array> data,
                   const WriterOptions& writer_options) {
-  ArrowArray arr;
-  ICEBERG_ARROW_RETURN_NOT_OK(::arrow::ExportArray(*data, &arr));
-
   ICEBERG_ASSIGN_OR_RAISE(
       auto writer, WriterFactoryRegistry::Open(FileFormatType::kParquet, writer_options));
-  {
-    internal::ArrowArrayGuard guard(&arr);
-    ICEBERG_RETURN_UNEXPECTED(writer->Write(arr));
-  }
+  ArrowArray arr;
+  ICEBERG_ARROW_RETURN_NOT_OK(::arrow::ExportArray(*data, &arr));
+  ICEBERG_RETURN_UNEXPECTED(writer->Write(arr));
   return writer->Close();
 }
 
@@ -323,13 +318,10 @@ TEST_F(ParquetReadWrite, SimpleStructRoundTrip) {
   ASSERT_THAT(ToArrowSchema(*schema, &arrow_c_schema), IsOk());
   auto arrow_schema = ::arrow::ImportType(&arrow_c_schema).ValueOrDie();
 
-  auto array =
-      ::arrow::json::ArrayFromJSONString(::arrow::struct_(arrow_schema->fields()),
-                                         R"([[{"a1": 1, "a2": "abc"}],
-                                                  [{"a1": 0}],
-                                                  [{"a2": "edf"}],
-                                                  [{}]])")
-          .ValueOrDie();
+  auto array = ::arrow::json::ArrayFromJSONString(
+                   ::arrow::struct_(arrow_schema->fields()),
+                   R"([[{"a1": 1, "a2": "abc"}], [{"a1": 0}], [{"a2": "edf"}], [{}]])")
+                   .ValueOrDie();
 
   std::shared_ptr<::arrow::Array> out;
   DoRoundtrip(array, schema, &out);
@@ -353,12 +345,11 @@ TEST_F(ParquetReadWrite, SimpleTypeRoundTrip) {
   ASSERT_THAT(ToArrowSchema(*schema, &arrow_c_schema), IsOk());
   auto arrow_schema = ::arrow::ImportType(&arrow_c_schema).ValueOrDie();
 
-  auto array = ::arrow::json::ArrayFromJSONString(
-                   ::arrow::struct_(arrow_schema->fields()),
-                   R"([[true, 1, 2, 1.1, 1.2, "abc", 44614000, 1756696503000000],
-                            [false, 0, 0, 0, 0, "", 0, 0],
-                            [null, null, null, null, null, null, null, null]])")
-                   .ValueOrDie();
+  auto array =
+      ::arrow::json::ArrayFromJSONString(
+          ::arrow::struct_(arrow_schema->fields()),
+          R"([[true, 1, 2, 1.1, 1.2, "abc", 44614000, 1756696503000000], [false, 0, 0, 0, 0, "", 0, 0], [null, null, null, null, null, null, null, null]])")
+          .ValueOrDie();
 
   std::shared_ptr<::arrow::Array> out;
   DoRoundtrip(array, schema, &out);