[GitHub] [arrow] wgtmac commented on a diff in pull request #34616: GH-29238 [C++][Dataset][Parquet] Support parquet modular encryption in the new Dataset API

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #34616:
URL: https://github.com/apache/arrow/pull/34616#discussion_r1186634927


##########
cpp/src/arrow/dataset/file_parquet.h:
##########
@@ -136,6 +137,32 @@ class ARROW_DS_EXPORT ParquetFileFormat : public FileFormat {
       fs::FileLocator destination_locator) const override;
 
   std::shared_ptr<FileWriteOptions> DefaultWriteOptions() override;
+
+  /// \brief A getter function to retrie‰ve the dataset encryption configuration
+  std::shared_ptr<DatasetEncryptionConfiguration> GetDatasetEncryptionConfig() const {
+    return dataset_encryption_config_;
+  }
+  /// \brief A getter function to retrieve the dataset decryption configuration
+  std::shared_ptr<DatasetDecryptionConfiguration> GetDatasetDecryptionConfig() const {
+    return dataset_decryption_config_;
+  }
+  /// \brief A setter for DatasetEncryptionConfiguration
+  void SetDatasetEncryptionConfig(
+      std::shared_ptr<DatasetEncryptionConfiguration> dataset_encryption_config) {
+    dataset_encryption_config_ = dataset_encryption_config;
+  }
+  /// \brief A setter for DatasetDecryptionConfiguration
+  void SetDatasetDecryptionConfig(
+      std::shared_ptr<DatasetDecryptionConfiguration> dataset_decryption_config) {
+    dataset_decryption_config_ = dataset_decryption_config;

Review Comment:
   ```suggestion
       dataset_decryption_config_ = std::move(dataset_decryption_config);
   ```



##########
cpp/src/arrow/dataset/dataset_encryption_test.cc:
##########
@@ -0,0 +1,391 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <string_view>
+#include "arrow/api.h"
+#include "arrow/array/builder_primitive.h"
+#include "arrow/builder.h"
+#include "arrow/dataset/api.h"
+#include "arrow/dataset/parquet_encryption_config.h"
+#include "arrow/dataset/partition.h"
+#include "arrow/filesystem/mockfs.h"
+#include "arrow/io/api.h"
+#include "arrow/status.h"
+#include "arrow/table.h"
+#include "arrow/testing/gtest_util.h"
+#include "gtest/gtest.h"
+#include "parquet/arrow/reader.h"
+#include "parquet/encryption/test_in_memory_kms.h"
+
+constexpr std::string_view dsFooterMasterKey = "0123456789012345";
+constexpr std::string_view dsFooterMasterKeyId = "footer_key";
+constexpr std::string_view dsColumnMasterKeys[] = {"1234567890123450"};
+constexpr std::string_view dsColumnMasterKeyIds[] = {"col_key"};
+const int dsNumColumns = 1;
+namespace arrow {
+namespace dataset {
+
+class DatasetEncryptionTest : public ::testing::Test {
+ protected:
+  // setup the test
+  void SetUp() {}
+
+  // Create our parquetfileformat with encryption properties
+  std::shared_ptr<ParquetFileFormat> CreateFileFormat(
+      const std::string_view* column_ids, const std::string_view* column_keys,
+      int num_columns, const std::string_view& footer_id,
+      const std::string_view& footer_key,
+      const std::string_view& footer_key_name = "footer_key",
+      const std::string_view& column_key_mapping = "col_key: a") {
+    auto key_list =
+        BuildKeyMap(column_ids, column_keys, num_columns, footer_id, footer_key);
+
+    auto crypto_factory = SetupCryptoFactory(true, key_list);
+    ::parquet::encryption::KmsConnectionConfig kms_connection_config;
+    auto encryption_config =
+        std::make_shared<::parquet::encryption::EncryptionConfiguration>(
+            std::string(footer_key_name));
+    encryption_config->column_keys = column_key_mapping;
+    if (footer_key_name.size() > 0) {
+      encryption_config->footer_key = footer_key_name;
+    }
+
+    DatasetEncryptionConfiguration dataset_encryption_config{
+        crypto_factory,
+        std::make_shared<parquet::encryption::KmsConnectionConfig>(kms_connection_config),

Review Comment:
   It seems `kms_connection_config` and `decryption_config` are using default values. It may be much easier to use if user does not have to pass a default value.



##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -67,8 +66,23 @@ parquet::ReaderProperties MakeReaderProperties(
     properties.disable_buffered_stream();
   }
   properties.set_buffer_size(parquet_scan_options->reader_properties->buffer_size());
+
+  std::shared_ptr<DatasetDecryptionConfiguration> dataset_decrypt_config =
+      format.GetDatasetDecryptionConfig();
+
+  if (dataset_decrypt_config != nullptr) {
+    auto file_decryption_prop =
+        dataset_decrypt_config->crypto_factory->GetFileDecryptionProperties(
+            *dataset_decrypt_config->kms_connection_config.get(),
+            *dataset_decrypt_config->decryption_config.get(), path, filesystem);
+
+    parquet_scan_options->reader_properties->file_decryption_properties(
+        file_decryption_prop);

Review Comment:
   ```suggestion
           std::move(file_decryption_prop));
   ```



##########
cpp/src/arrow/dataset/file_parquet.h:
##########
@@ -136,6 +137,32 @@ class ARROW_DS_EXPORT ParquetFileFormat : public FileFormat {
       fs::FileLocator destination_locator) const override;
 
   std::shared_ptr<FileWriteOptions> DefaultWriteOptions() override;
+
+  /// \brief A getter function to retrie‰ve the dataset encryption configuration
+  std::shared_ptr<DatasetEncryptionConfiguration> GetDatasetEncryptionConfig() const {
+    return dataset_encryption_config_;
+  }
+  /// \brief A getter function to retrieve the dataset decryption configuration
+  std::shared_ptr<DatasetDecryptionConfiguration> GetDatasetDecryptionConfig() const {
+    return dataset_decryption_config_;
+  }
+  /// \brief A setter for DatasetEncryptionConfiguration
+  void SetDatasetEncryptionConfig(
+      std::shared_ptr<DatasetEncryptionConfiguration> dataset_encryption_config) {
+    dataset_encryption_config_ = dataset_encryption_config;

Review Comment:
   ```suggestion
       dataset_encryption_config_ = std::move(dataset_encryption_config);
   ```



##########
cpp/src/arrow/dataset/file_parquet.h:
##########
@@ -136,6 +137,32 @@ class ARROW_DS_EXPORT ParquetFileFormat : public FileFormat {
       fs::FileLocator destination_locator) const override;
 
   std::shared_ptr<FileWriteOptions> DefaultWriteOptions() override;
+
+  /// \brief A getter function to retrie‰ve the dataset encryption configuration

Review Comment:
   ```suggestion
     /// \brief A getter function to retrieve the dataset encryption configuration
   ```



##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -594,10 +608,34 @@ Result<std::shared_ptr<FileWriter>> ParquetFileFormat::MakeWriter(
   auto parquet_options = checked_pointer_cast<ParquetFileWriteOptions>(options);
 
   std::unique_ptr<parquet::arrow::FileWriter> parquet_writer;
-  ARROW_ASSIGN_OR_RAISE(parquet_writer, parquet::arrow::FileWriter::Open(
-                                            *schema, default_memory_pool(), destination,
-                                            parquet_options->writer_properties,
-                                            parquet_options->arrow_writer_properties));
+
+  std::shared_ptr<DatasetEncryptionConfiguration> dataset_encrypt_config =
+      GetDatasetEncryptionConfig();
+
+  if (dataset_encrypt_config != nullptr) {
+    auto file_encryption_prop =
+        dataset_encrypt_config->crypto_factory->GetFileEncryptionProperties(
+            *dataset_encrypt_config->kms_connection_config.get(),
+            *dataset_encrypt_config->encryption_config.get(), destination_locator.path,
+            destination_locator.filesystem);
+
+    auto writer_properties =
+        parquet::WriterProperties::Builder(*parquet_options->writer_properties.get())
+            .encryption(file_encryption_prop)

Review Comment:
   ```suggestion
               .encryption(std:move(file_encryption_prop))
   ```



##########
cpp/src/arrow/dataset/dataset_encryption_test.cc:
##########
@@ -0,0 +1,391 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <string_view>
+#include "arrow/api.h"
+#include "arrow/array/builder_primitive.h"
+#include "arrow/builder.h"
+#include "arrow/dataset/api.h"
+#include "arrow/dataset/parquet_encryption_config.h"
+#include "arrow/dataset/partition.h"
+#include "arrow/filesystem/mockfs.h"
+#include "arrow/io/api.h"
+#include "arrow/status.h"
+#include "arrow/table.h"
+#include "arrow/testing/gtest_util.h"
+#include "gtest/gtest.h"
+#include "parquet/arrow/reader.h"
+#include "parquet/encryption/test_in_memory_kms.h"
+
+constexpr std::string_view dsFooterMasterKey = "0123456789012345";

Review Comment:
   We may need to use snake case naming here.



##########
cpp/src/arrow/dataset/dataset_encryption_test.cc:
##########
@@ -0,0 +1,391 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <string_view>
+#include "arrow/api.h"
+#include "arrow/array/builder_primitive.h"
+#include "arrow/builder.h"
+#include "arrow/dataset/api.h"
+#include "arrow/dataset/parquet_encryption_config.h"
+#include "arrow/dataset/partition.h"
+#include "arrow/filesystem/mockfs.h"
+#include "arrow/io/api.h"
+#include "arrow/status.h"
+#include "arrow/table.h"
+#include "arrow/testing/gtest_util.h"
+#include "gtest/gtest.h"
+#include "parquet/arrow/reader.h"
+#include "parquet/encryption/test_in_memory_kms.h"
+
+constexpr std::string_view dsFooterMasterKey = "0123456789012345";
+constexpr std::string_view dsFooterMasterKeyId = "footer_key";
+constexpr std::string_view dsColumnMasterKeys[] = {"1234567890123450"};
+constexpr std::string_view dsColumnMasterKeyIds[] = {"col_key"};
+const int dsNumColumns = 1;
+namespace arrow {
+namespace dataset {
+
+class DatasetEncryptionTest : public ::testing::Test {
+ protected:
+  // setup the test
+  void SetUp() {}
+
+  // Create our parquetfileformat with encryption properties
+  std::shared_ptr<ParquetFileFormat> CreateFileFormat(
+      const std::string_view* column_ids, const std::string_view* column_keys,
+      int num_columns, const std::string_view& footer_id,
+      const std::string_view& footer_key,
+      const std::string_view& footer_key_name = "footer_key",
+      const std::string_view& column_key_mapping = "col_key: a") {

Review Comment:
   ```suggestion
         int num_columns, std::string_view footer_id,
         std::string_view footer_key,
         std::string_view footer_key_name = "footer_key",
         std::string_view column_key_mapping = "col_key: a") {
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org