fix some review comments

Author: HuaHuaY

cmake_modules/IcebergBuildUtils.cmake

@@ -131,7 +131,7 @@ function(add_iceberg_lib LIB_NAME)
     endif()
 
     if(LIB_INCLUDES)
-      target_include_directories(${LIB_NAME}_shared SYSTEM PUBLIC ${ARG_EXTRA_INCLUDES})
+      target_include_directories(${LIB_NAME}_shared PUBLIC ${ARG_EXTRA_INCLUDES})
     endif()
 
     if(ARG_PRIVATE_INCLUDES)
@@ -179,7 +179,7 @@ function(add_iceberg_lib LIB_NAME)
     endif()
 
     if(LIB_INCLUDES)
-      target_include_directories(${LIB_NAME}_static SYSTEM PUBLIC ${ARG_EXTRA_INCLUDES})
+      target_include_directories(${LIB_NAME}_static PUBLIC ${ARG_EXTRA_INCLUDES})
     endif()
 
     if(ARG_PRIVATE_INCLUDES)

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/parquet_test.cc

@@ -26,19 +26,19 @@
 #include <arrow/table.h>
 #include <arrow/type.h>
 #include <arrow/util/key_value_metadata.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"
@@ -318,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);
@@ -348,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);