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

Jump to bottom

[C++] read encrypted ORC file #1993

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[C++] read encrypted ORC file #1993

wants to merge 4 commits into from

Conversation

zxf216
Copy link

@zxf216 zxf216 commented Aug 4, 2024 •
edited
Loading

What changes were proposed in this pull request?

you can use C++ version code to read encrypted ORC files
ORC file structure：links

Why are the changes needed?

The C++ version is not yet able to read encrypted ORC files now

How was this patch tested?

orc::RowReaderOptions rowReaderOptions;
orc::ReaderOptions readerOpts;
std::shared_ptr<orc::InMemoryKeystore> keyStore = std::make_shared<orc::InMemoryKeystore>();
std::string passKey ="secret123";
keyStore->addKey("pii", 0, orc::EncryptionAlgorithm::AES_CTR_128,passKey);
readerOpts.setKeyProvider(keyStore);
printContents("/root/encryption.orc",readerOpts, rowReaderOptions);

@github-actions github-actions bot added the CPP label Aug 4, 2024
dongjoon-hyun
dongjoon-hyun reviewed Aug 4, 2024
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR, @zxf216 .

We have been tracking this via

BTW, could you make the CI happy by fixing all failures?

@wgtmac
Copy link
Member

wgtmac commented Aug 5, 2024

Thanks @zxf216! Let me take a first pass. cc @coderex2522 @ffacs

@zxf216
Copy link
Author

zxf216 commented Aug 6, 2024

Thank you for making a PR, @zxf216 .

We have been tracking this via

BTW, could you make the CI happy by fixing all failures?
Okay

@dongjoon-hyun dongjoon-hyun modified the milestone: 2.1.0 Aug 11, 2024
@dongjoon-hyun dongjoon-hyun marked this pull request as draft August 11, 2024 18:32
@dongjoon-hyun
Copy link
Member

I converted it to Draft because CIs fails still.

wgtmac
wgtmac requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

Thanks @zxf216 again for the excellent work! I just took a preliminary look and have left some inline comments. As you can see that we have various CI jobs for test cases, code format, license check, etc. I strongly recommend splitting the PR into smaller ones to make the review process easier.

c++/include/orc/Common.hh Outdated Show resolved Hide resolved
c++/include/orc/Common.hh Outdated Show resolved Hide resolved
c++/include/orc/Common.hh Outdated Show resolved Hide resolved
c++/src/CMakeLists.txt Show resolved Hide resolved
c++/src/TypeImpl.cc
@@ -837,5 +837,4 @@ namespace orc {
}
return nullptr;
}

Copy link
Member

Choose a reason for hiding this comment

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

Please revert the unnecessary change.

c++/include/orc/Reader.hh
@@ -39,7 +39,7 @@ namespace orc {
// classes that hold data members so we can maintain binary compatibility
struct ReaderOptionsPrivate;
struct RowReaderOptionsPrivate;

class KeyProvider;
Copy link
Member

Choose a reason for hiding this comment

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

The c++/include/orc folder is public and files in it will be installed. If the KeyProvider is also public, please move its definition to the include folder as well. Otherwise, we should remove the related function from here.

c++/src/security/InMemoryKeystore.cc
@@ -0,0 +1,247 @@
// Copyright 2010-present vivo, Inc. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Please note that we have a license check which does not allow custom modification to the license header in this repo. I'm not sure if this violates any ASF rule to add something like this. @dongjoon-hyun Do you have any concern?

c++/src/security/ReaderEncryption.hh
std::shared_ptr<SecretKeySpec> getStripeKey(long stripe);
};

// 加密变体，每个列一个
Copy link
Member

Choose a reason for hiding this comment

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

Please use English for comment.

@dongjoon-hyun dongjoon-hyun changed the title read encrypted ORC file [C++] read encrypted ORC file Sep 20, 2024
@dongjoon-hyun
Copy link
Member

Gentle ping, @zxf216 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@wgtmac wgtmac wgtmac requested changes

Assignees
No one assigned
Labels
CPP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zxf216 @wgtmac @dongjoon-hyun