Skip to content

Commit 0bd4e73

Browse files
authored
GH-37891: [C++][Parquet] Refine several classes in Parquet encryption (#46202)
### What changes are included in this PR? * Pass movable objects by value to functions that keep a copy of them. * Use emplace() instead of insert() when the arguments can be moved safely. * Remove unnecessary const qualifier on member variables that makes the class non-copyable and non-movable. * Remove unnecessary constructors and destructors that can be generated automatically by compilers. ### Are these changes tested? Yes, by existing tests. ### Are there any user-facing changes? No. * GitHub Issue: #37891 Authored-by: Eddie Chang <kalcifer7319@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent 01e8a60 commit 0bd4e73

11 files changed

Lines changed: 136 additions & 156 deletions

cpp/examples/parquet/low_level_api/encryption_reader_writer.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ int main(int argc, char** argv) {
8181
parquet::WriterProperties::Builder builder;
8282
// Add the current encryption configuration to WriterProperties.
8383
builder.encryption(file_encryption_builder.footer_key_metadata("kf")
84-
->encrypted_columns(encryption_cols)
84+
->encrypted_columns(std::move(encryption_cols))
8585
->build());
8686

8787
// Add other writer properties
@@ -216,7 +216,7 @@ int main(int argc, char** argv) {
216216

217217
// Add the current decryption configuration to ReaderProperties.
218218
reader_properties.file_decryption_properties(
219-
file_decryption_builder.key_retriever(kr1)->build());
219+
file_decryption_builder.key_retriever(std::move(kr1))->build());
220220

221221
// Create a ParquetReader instance
222222
std::unique_ptr<parquet::ParquetFileReader> parquet_reader =

cpp/examples/parquet/low_level_api/encryption_reader_writer_all_crypto_options.cc

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ void InteropTestWriteEncryptedParquetFiles(std::string root_path) {
185185

186186
vector_of_encryption_configurations.push_back(
187187
file_encryption_builder_2.footer_key_metadata("kf")
188-
->encrypted_columns(encryption_cols2)
188+
->encrypted_columns(std::move(encryption_cols2))
189189
->build());
190190

191191
// Encryption configuration 3: Encrypt two columns, with different keys.
@@ -205,7 +205,7 @@ void InteropTestWriteEncryptedParquetFiles(std::string root_path) {
205205

206206
vector_of_encryption_configurations.push_back(
207207
file_encryption_builder_3.footer_key_metadata("kf")
208-
->encrypted_columns(encryption_cols3)
208+
->encrypted_columns(std::move(encryption_cols3))
209209
->set_plaintext_footer()
210210
->build());
211211

@@ -225,7 +225,7 @@ void InteropTestWriteEncryptedParquetFiles(std::string root_path) {
225225

226226
vector_of_encryption_configurations.push_back(
227227
file_encryption_builder_4.footer_key_metadata("kf")
228-
->encrypted_columns(encryption_cols4)
228+
->encrypted_columns(std::move(encryption_cols4))
229229
->aad_prefix(fileName)
230230
->build());
231231

@@ -244,7 +244,7 @@ void InteropTestWriteEncryptedParquetFiles(std::string root_path) {
244244
kFooterEncryptionKey);
245245

246246
vector_of_encryption_configurations.push_back(
247-
file_encryption_builder_5.encrypted_columns(encryption_cols5)
247+
file_encryption_builder_5.encrypted_columns(std::move(encryption_cols5))
248248
->footer_key_metadata("kf")
249249
->aad_prefix(fileName)
250250
->disable_aad_prefix_storage()
@@ -266,7 +266,7 @@ void InteropTestWriteEncryptedParquetFiles(std::string root_path) {
266266

267267
vector_of_encryption_configurations.push_back(
268268
file_encryption_builder_6.footer_key_metadata("kf")
269-
->encrypted_columns(encryption_cols6)
269+
->encrypted_columns(std::move(encryption_cols6))
270270
->algorithm(parquet::ParquetCipher::AES_GCM_CTR_V1)
271271
->build());
272272

@@ -373,7 +373,7 @@ void InteropTestReadEncryptedParquetFiles(std::string root_path) {
373373

374374
parquet::FileDecryptionProperties::Builder file_decryption_builder_1;
375375
vector_of_decryption_configurations.push_back(
376-
file_decryption_builder_1.key_retriever(kr1)->build());
376+
file_decryption_builder_1.key_retriever(std::move(kr1))->build());
377377

378378
// Decryption configuration 2: Decrypt using key retriever callback that holds the keys
379379
// of two encrypted columns and the footer key. Supply aad_prefix.
@@ -387,7 +387,9 @@ void InteropTestReadEncryptedParquetFiles(std::string root_path) {
387387

388388
parquet::FileDecryptionProperties::Builder file_decryption_builder_2;
389389
vector_of_decryption_configurations.push_back(
390-
file_decryption_builder_2.key_retriever(kr2)->aad_prefix(fileName)->build());
390+
file_decryption_builder_2.key_retriever(std::move(kr2))
391+
->aad_prefix(fileName)
392+
->build());
391393

392394
// Decryption configuration 3: Decrypt using explicit column and footer keys.
393395
std::string path_double = "double_field";
@@ -405,7 +407,7 @@ void InteropTestReadEncryptedParquetFiles(std::string root_path) {
405407
parquet::FileDecryptionProperties::Builder file_decryption_builder_3;
406408
vector_of_decryption_configurations.push_back(
407409
file_decryption_builder_3.footer_key(kFooterEncryptionKey)
408-
->column_keys(decryption_cols)
410+
->column_keys(std::move(decryption_cols))
409411
->build());
410412

411413
/**********************************************************************************

cpp/src/parquet/encryption/crypto_factory.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,13 @@ std::shared_ptr<FileEncryptionProperties> CryptoFactory::GetFileEncryptionProper
7979

8080
FileEncryptionProperties::Builder properties_builder =
8181
FileEncryptionProperties::Builder(footer_key);
82-
properties_builder.footer_key_metadata(footer_key_metadata);
82+
properties_builder.footer_key_metadata(std::move(footer_key_metadata));
8383
properties_builder.algorithm(encryption_config.encryption_algorithm);
8484

8585
if (!encryption_config.uniform_encryption) {
8686
ColumnPathToEncryptionPropertiesMap encrypted_columns =
8787
GetColumnEncryptionProperties(dek_length, column_key_str, &key_wrapper);
88-
properties_builder.encrypted_columns(encrypted_columns);
88+
properties_builder.encrypted_columns(std::move(encrypted_columns));
8989

9090
if (encryption_config.plaintext_footer) {
9191
properties_builder.set_plaintext_footer();
@@ -175,7 +175,7 @@ std::shared_ptr<FileDecryptionProperties> CryptoFactory::GetFileDecryptionProper
175175
file_path, file_system);
176176

177177
return FileDecryptionProperties::Builder()
178-
.key_retriever(key_retriever)
178+
.key_retriever(std::move(key_retriever))
179179
->plaintext_files_allowed()
180180
->build();
181181
}

cpp/src/parquet/encryption/encryption.cc

Lines changed: 54 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -54,20 +54,19 @@ ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key(
5454
if (column_key.empty()) return this;
5555

5656
DCHECK(key_.empty());
57-
key_ = column_key;
57+
key_ = std::move(column_key);
5858
return this;
5959
}
6060

6161
ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key_metadata(
62-
const std::string& key_metadata) {
62+
std::string key_metadata) {
6363
DCHECK(!key_metadata.empty());
64-
DCHECK(key_metadata_.empty());
65-
key_metadata_ = key_metadata;
64+
key_metadata_ = std::move(key_metadata);
6665
return this;
6766
}
6867

6968
ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key_id(
70-
const std::string& key_id) {
69+
std::string key_id) {
7170
// key_id is expected to be in UTF8 encoding
7271
::arrow::util::InitializeUTF8();
7372
const uint8_t* data = reinterpret_cast<const uint8_t*>(key_id.c_str());
@@ -76,47 +75,47 @@ ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key_id
7675
}
7776

7877
DCHECK(!key_id.empty());
79-
this->key_metadata(key_id);
78+
this->key_metadata(std::move(key_id));
8079
return this;
8180
}
8281

8382
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::column_keys(
84-
const ColumnPathToDecryptionPropertiesMap& column_decryption_properties) {
83+
ColumnPathToDecryptionPropertiesMap column_decryption_properties) {
8584
if (column_decryption_properties.size() == 0) return this;
8685

8786
if (column_decryption_properties_.size() != 0)
8887
throw ParquetException("Column properties already set");
8988

90-
column_decryption_properties_ = column_decryption_properties;
89+
column_decryption_properties_ = std::move(column_decryption_properties);
9190
return this;
9291
}
9392

9493
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::footer_key(
95-
const std::string footer_key) {
94+
std::string footer_key) {
9695
if (footer_key.empty()) {
9796
return this;
9897
}
9998
DCHECK(footer_key_.empty());
100-
footer_key_ = footer_key;
99+
footer_key_ = std::move(footer_key);
101100
return this;
102101
}
103102

104103
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::key_retriever(
105-
const std::shared_ptr<DecryptionKeyRetriever>& key_retriever) {
104+
std::shared_ptr<DecryptionKeyRetriever> key_retriever) {
106105
if (key_retriever == nullptr) return this;
107106

108107
DCHECK(key_retriever_ == nullptr);
109-
key_retriever_ = key_retriever;
108+
key_retriever_ = std::move(key_retriever);
110109
return this;
111110
}
112111

113112
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix(
114-
const std::string& aad_prefix) {
113+
std::string aad_prefix) {
115114
if (aad_prefix.empty()) {
116115
return this;
117116
}
118117
DCHECK(aad_prefix_.empty());
119-
aad_prefix_ = aad_prefix;
118+
aad_prefix_ = std::move(aad_prefix);
120119
return this;
121120
}
122121

@@ -130,11 +129,11 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix
130129
}
131130

132131
ColumnDecryptionProperties::Builder* ColumnDecryptionProperties::Builder::key(
133-
const std::string& key) {
132+
std::string key) {
134133
if (key.empty()) return this;
135134

136135
DCHECK(!key.empty());
137-
key_ = key;
136+
key_ = std::move(key);
138137
return this;
139138
}
140139

@@ -144,31 +143,31 @@ std::shared_ptr<ColumnDecryptionProperties> ColumnDecryptionProperties::Builder:
144143
}
145144

146145
FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::footer_key_metadata(
147-
const std::string& footer_key_metadata) {
146+
std::string footer_key_metadata) {
148147
if (footer_key_metadata.empty()) return this;
149148

150149
DCHECK(footer_key_metadata_.empty());
151-
footer_key_metadata_ = footer_key_metadata;
150+
footer_key_metadata_ = std::move(footer_key_metadata);
152151
return this;
153152
}
154153

155154
FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::encrypted_columns(
156-
const ColumnPathToEncryptionPropertiesMap& encrypted_columns) {
155+
ColumnPathToEncryptionPropertiesMap encrypted_columns) {
157156
if (encrypted_columns.size() == 0) return this;
158157

159158
if (encrypted_columns_.size() != 0)
160159
throw ParquetException("Column properties already set");
161160

162-
encrypted_columns_ = encrypted_columns;
161+
encrypted_columns_ = std::move(encrypted_columns);
163162
return this;
164163
}
165164

166165
FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::aad_prefix(
167-
const std::string& aad_prefix) {
166+
std::string aad_prefix) {
168167
if (aad_prefix.empty()) return this;
169168

170169
DCHECK(aad_prefix_.empty());
171-
aad_prefix_ = aad_prefix;
170+
aad_prefix_ = std::move(aad_prefix);
172171
store_aad_prefix_in_file_ = true;
173172
return this;
174173
}
@@ -182,11 +181,11 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
182181
}
183182

184183
ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
185-
const std::string& column_path,
186-
const std::string& key,
187-
const std::string& key_metadata)
188-
: column_path_(column_path) {
184+
std::string column_path,
185+
std::string key,
186+
std::string key_metadata) {
189187
DCHECK(!column_path.empty());
188+
column_path_ = std::move(column_path);
190189
if (!encrypted) {
191190
DCHECK(key.empty() && key_metadata.empty());
192191
}
@@ -201,20 +200,20 @@ ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
201200
}
202201

203202
encrypted_ = encrypted;
204-
key_metadata_ = key_metadata;
205-
key_ = key;
203+
key_metadata_ = std::move(key_metadata);
204+
key_ = std::move(key);
206205
}
207206

208-
ColumnDecryptionProperties::ColumnDecryptionProperties(const std::string& column_path,
209-
const std::string& key)
210-
: column_path_(column_path) {
207+
ColumnDecryptionProperties::ColumnDecryptionProperties(std::string column_path,
208+
std::string key) {
211209
DCHECK(!column_path.empty());
210+
column_path_ = std::move(column_path);
212211

213212
if (!key.empty()) {
214213
DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32);
215214
}
216215

217-
key_ = key;
216+
key_ = std::move(key);
218217
}
219218

220219
std::string FileDecryptionProperties::column_key(const std::string& column_path) const {
@@ -225,14 +224,14 @@ std::string FileDecryptionProperties::column_key(const std::string& column_path)
225224
return column_prop->key();
226225
}
227226
}
228-
return empty_string_;
227+
return {};
229228
}
230229

231230
FileDecryptionProperties::FileDecryptionProperties(
232-
const std::string& footer_key, std::shared_ptr<DecryptionKeyRetriever> key_retriever,
233-
bool check_plaintext_footer_integrity, const std::string& aad_prefix,
231+
std::string footer_key, std::shared_ptr<DecryptionKeyRetriever> key_retriever,
232+
bool check_plaintext_footer_integrity, std::string aad_prefix,
234233
std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier,
235-
const ColumnPathToDecryptionPropertiesMap& column_decryption_properties,
234+
ColumnPathToDecryptionPropertiesMap column_decryption_properties,
236235
bool plaintext_files_allowed) {
237236
DCHECK(!footer_key.empty() || nullptr != key_retriever ||
238237
0 != column_decryption_properties.size());
@@ -245,16 +244,16 @@ FileDecryptionProperties::FileDecryptionProperties(
245244
DCHECK(nullptr != key_retriever);
246245
}
247246
aad_prefix_verifier_ = std::move(aad_prefix_verifier);
248-
footer_key_ = footer_key;
247+
footer_key_ = std::move(footer_key);
249248
check_plaintext_footer_integrity_ = check_plaintext_footer_integrity;
250249
key_retriever_ = std::move(key_retriever);
251-
aad_prefix_ = aad_prefix;
252-
column_decryption_properties_ = column_decryption_properties;
250+
aad_prefix_ = std::move(aad_prefix);
251+
column_decryption_properties_ = std::move(column_decryption_properties);
253252
plaintext_files_allowed_ = plaintext_files_allowed;
254253
}
255254

256255
FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::footer_key_id(
257-
const std::string& key_id) {
256+
std::string key_id) {
258257
// key_id is expected to be in UTF8 encoding
259258
::arrow::util::InitializeUTF8();
260259
const uint8_t* data = reinterpret_cast<const uint8_t*>(key_id.c_str());
@@ -266,7 +265,7 @@ FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::footer_key
266265
return this;
267266
}
268267

269-
return footer_key_metadata(key_id);
268+
return footer_key_metadata(std::move(key_id));
270269
}
271270

272271
std::shared_ptr<ColumnEncryptionProperties>
@@ -283,38 +282,37 @@ FileEncryptionProperties::column_encryption_properties(const std::string& column
283282
}
284283

285284
FileEncryptionProperties::FileEncryptionProperties(
286-
ParquetCipher::type cipher, const std::string& footer_key,
287-
const std::string& footer_key_metadata, bool encrypted_footer,
288-
const std::string& aad_prefix, bool store_aad_prefix_in_file,
289-
const ColumnPathToEncryptionPropertiesMap& encrypted_columns)
290-
: footer_key_(footer_key),
291-
footer_key_metadata_(footer_key_metadata),
285+
ParquetCipher::type cipher, std::string footer_key, std::string footer_key_metadata,
286+
bool encrypted_footer, std::string aad_prefix, bool store_aad_prefix_in_file,
287+
ColumnPathToEncryptionPropertiesMap encrypted_columns)
288+
: footer_key_(std::move(footer_key)),
289+
footer_key_metadata_(std::move(footer_key_metadata)),
292290
encrypted_footer_(encrypted_footer),
293-
aad_prefix_(aad_prefix),
291+
aad_prefix_(std::move(aad_prefix)),
294292
store_aad_prefix_in_file_(store_aad_prefix_in_file),
295-
encrypted_columns_(encrypted_columns) {
296-
DCHECK(!footer_key.empty());
293+
encrypted_columns_(std::move(encrypted_columns)) {
294+
DCHECK(!footer_key_.empty());
297295
// footer_key must be either 16, 24 or 32 bytes.
298-
DCHECK(footer_key.length() == 16 || footer_key.length() == 24 ||
299-
footer_key.length() == 32);
296+
DCHECK(footer_key_.length() == 16 || footer_key_.length() == 24 ||
297+
footer_key_.length() == 32);
300298

301299
uint8_t aad_file_unique[kAadFileUniqueLength];
302300
encryption::RandBytes(aad_file_unique, kAadFileUniqueLength);
303301
std::string aad_file_unique_str(reinterpret_cast<char const*>(aad_file_unique),
304302
kAadFileUniqueLength);
305303

306304
bool supply_aad_prefix = false;
307-
if (aad_prefix.empty()) {
305+
if (aad_prefix_.empty()) {
308306
file_aad_ = aad_file_unique_str;
309307
} else {
310-
file_aad_ = aad_prefix + aad_file_unique_str;
308+
file_aad_ = aad_prefix_ + aad_file_unique_str;
311309
if (!store_aad_prefix_in_file_) supply_aad_prefix = true;
312310
}
313311
algorithm_.algorithm = cipher;
314312
algorithm_.aad.aad_file_unique = aad_file_unique_str;
315313
algorithm_.aad.supply_aad_prefix = supply_aad_prefix;
316-
if (!aad_prefix.empty() && store_aad_prefix_in_file_) {
317-
algorithm_.aad.aad_prefix = aad_prefix;
314+
if (!aad_prefix_.empty() && store_aad_prefix_in_file_) {
315+
algorithm_.aad.aad_prefix = aad_prefix_;
318316
}
319317
}
320318

0 commit comments

Comments
 (0)