Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-41431: [C++][Parquet][Dataset] Fix repeated scan on encrypted dataset #41550

Merged
merged 2 commits into from
May 8, 2024
Merged

GH-41431: [C++][Parquet][Dataset] Fix repeated scan on encrypted dataset #41550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f442528
GH-41431: [C++][Parquet][Dataset] Fix repeated scan on encrypted dataset
wgtmac May 6, 2024
0f0253f
use file_decryptor from FileMetaData
wgtmac May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions cpp/src/arrow/dataset/file_parquet_encryption_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,22 @@ class DatasetEncryptionTestBase : public ::testing::Test {
FileSystemDatasetFactory::Make(file_system_, selector,
file_format, factory_options));

// Read dataset into table
// Create the dataset
ASSERT_OK_AND_ASSIGN(auto dataset, dataset_factory->Finish());
ASSERT_OK_AND_ASSIGN(auto scanner_builder, dataset->NewScan());
ASSERT_OK_AND_ASSIGN(auto scanner, scanner_builder->Finish());
ASSERT_OK_AND_ASSIGN(auto read_table, scanner->ToTable());

// Verify the data was read correctly
ASSERT_OK_AND_ASSIGN(auto combined_table, read_table->CombineChunks());
// Validate the table
ASSERT_OK(combined_table->ValidateFull());
AssertTablesEqual(*combined_table, *table_);

// Reuse the dataset above to scan it twice to make sure decryption works correctly.
for (size_t i = 0; i < 2; ++i) {
// Read dataset into table
ASSERT_OK_AND_ASSIGN(auto scanner_builder, dataset->NewScan());
ASSERT_OK_AND_ASSIGN(auto scanner, scanner_builder->Finish());
ASSERT_OK_AND_ASSIGN(auto read_table, scanner->ToTable());

// Verify the data was read correctly
ASSERT_OK_AND_ASSIGN(auto combined_table, read_table->CombineChunks());
// Validate the table
ASSERT_OK(combined_table->ValidateFull());
AssertTablesEqual(*combined_table, *table_);
}
}

protected:
Expand Down
1 change: 1 addition & 0 deletions cpp/src/parquet/file_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ class SerializedFile : public ParquetFileReader::Contents {

void set_metadata(std::shared_ptr<FileMetaData> metadata) {
file_metadata_ = std::move(metadata);
file_decryptor_ = file_metadata_->file_decryptor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing, here we set file_decryptor_ from file_metadata_, but in ParseUnencryptedFileMetadata and ParseMetaDataOfEncryptedFileWithPlaintextFooter we set file_metadata_ from file_decryptor_. Can we please make this consistent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/apache/arrow/blob/main/cpp/src/parquet/file_reader.cc#L775-L780

I think these are in two different directions while opening a parquet reader:

  • For ParseXXX functions where we parse footer to create file_decryptor_ and file_metadata_. We need to set file_decryptor_ to file_metadata_ so that both SerializedFile and FileMetaData have a copy of the decryptor.
  • For set_metadata function, we already have the cached FileMetaData but need to create SerializedFile where its file_decryptor_ is null. Therefore we need to get file_decryptor_ from file_metadata_.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this begs the question: do we need a file_decryptor_ field here? We could just get it from the metadata everytime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! Let me consolidate them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed file_decryptor_ from SerializedFile and SerializedRowGroup. Let me know WDYT. @pitrou

}

void PreBuffer(const std::vector<int>& row_groups,
Expand Down
8 changes: 8 additions & 0 deletions cpp/src/parquet/metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,10 @@ class FileMetaData::FileMetaDataImpl {
file_decryptor_ = std::move(file_decryptor);
}

std::shared_ptr<InternalFileDecryptor> file_decryptor() const {
return file_decryptor_;
}

private:
friend FileMetaDataBuilder;
uint32_t metadata_len_ = 0;
Expand Down Expand Up @@ -947,6 +951,10 @@ void FileMetaData::set_file_decryptor(
impl_->set_file_decryptor(std::move(file_decryptor));
}

std::shared_ptr<InternalFileDecryptor> FileMetaData::file_decryptor() const {
return impl_->file_decryptor();
}

ParquetVersion::type FileMetaData::version() const {
switch (impl_->version()) {
case 1:
Expand Down
1 change: 1 addition & 0 deletions cpp/src/parquet/metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ class PARQUET_EXPORT FileMetaData {
std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);

void set_file_decryptor(std::shared_ptr<InternalFileDecryptor> file_decryptor);
std::shared_ptr<InternalFileDecryptor> file_decryptor() const;

// PIMPL Idiom
FileMetaData();
Expand Down
Loading